Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: integrate ERC-4906 #568

Merged
merged 2 commits into from
Jun 28, 2023
Merged

feat: integrate ERC-4906 #568

merged 2 commits into from
Jun 28, 2023

Conversation

PaulRBerg
Copy link
Member

Closes #549 by integrating OpenZeppelin's IERC4906.

Function Event
cancel MetadataUpdate
renounce MetadataUpdate
setNFTDescriptor BatchMetadataUpdate
withdraw MetadataUpdate

Notes:

  • The rationale for emitting the MetadataUpdate event on every withdraw is thus: the state of the stream may change between withdrawals. We want to intercept every state-changing operation to make NFT marketplaces refresh our NFT SVGs.
    • Through this lens, we make the protocol users pay the cost of refreshing the NFT metadata - which is a fair requirement given that it plays to their advantage for the NFT SVG to be kept up to date.
  • The same rationale applies to renounce
  • I know that the _toTokenId passed to BatchMetadataUpdate is 0 when _nextStreamId is 1, but this should be a minor detail. I mean, one would hope that the _nextStreamId will not retain the value of 1 for long. Even in the worst-case scenario when this is a problem for NFT marketplaces, we can create a stream ourselves before calling setNFTDescriptor.

Copy link

@tibbarytsur tibbarytsur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly solves the problem except for the streamed amount/percentage between withdrawals. But I suppose the only way to handle that is the bot to trigger the relevant 3rd party APIs.

andreivladbrg

This comment was marked as resolved.

@PaulRBerg

This comment was marked as resolved.

@PaulRBerg PaulRBerg force-pushed the feat/erc-4906 branch 4 times, most recently from 203875a to c41198a Compare June 28, 2023 09:13
@PaulRBerg
Copy link
Member Author

@andreivladbrg is this PR ready to be merged now?

docs: polish wording in NatSpec
refactor: move "_nextStreamId" to "SablierV2Lockup"
test: test emit of ERC-4906 events
@PaulRBerg PaulRBerg merged commit c9228c2 into main Jun 28, 2023
9 checks passed
@PaulRBerg PaulRBerg deleted the feat/erc-4906 branch June 28, 2023 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emit ERC-4906 events to refresh NFT metadata
3 participants