feat: option to set transferability of NFTs when creating stream #668
Conversation
|
@PaulRBerg can you please review this PR? |
smol-ninja
changed the title
smol-ninja
changed the title
There was a problem hiding this comment.
Thanks, @smol-ninja. Feedback left below.
We need to add a new branch in the burn.tree test file to show that non-transferable streams can be burned.
I will let you do this.
refactor: remove "canStreamTransfer" refactor: remove double "r" in "transferrable" refactor: simplify implementation test: allow non-transferable streams in invariant tests test: dry tests for "isTransferable" test: improve function and test names
https://github.com/smol-ninja/v2-core/commit/699fafe9fb01e68c6c621823220057930d70d11f |
There was a problem hiding this comment.
Finished the review - great job, @smol-ninja! Lovely working with you.
We will merge the PR after the CI checks pass.
Thanks, the pleasure is mine too. I realized I should focus more on polishing the code so you don't have to refactor it. Are there any documented best code practices that you follow for this? For example, I see you use |
Yes! See this Twitter thread and our Notion document Coding Practices on Notion.
Yep, that is part of the Coding Practices. |
|
Fork tests are failing because you don't have the Mainnet URLs set in your fork of the repo. That's fine. |
|
I didn't know that it would use the keys from my fork. I have now added them to my repo for the future. |
Resolves #630.
It adds:
transferrableboolean parameter in the create stream parameter structs in "DataTypes.sol"._beforeTokenTransfer()in "src/SablierV2LockupDynamic.sol" and "src/SablierV2LockupLinear.sol".