Implement ERC721 standard in both Linear and Pro contracts #131

andreivladbrg · 2022-08-15T12:01:02Z

Install Solmate library
Inherit ERC721 in both contracts
Mint NFT in the create function in both contracts
Remove the recipient address from the Stream struct in both contracts
Add isApprovedOrOwner function that returns a bool value wether the caller is the owner of the stream or is approved
Change the onlySenderOrRecipient modifier to isAuthorizedForStream that calls isApprovedOrOwner function
Test the case when the caller is an approved third party in the cancel cancelAll withdraw withdrawAll withdrawAllTo withdrawTo functions
Test the case when the recipient is no longer the owner of the stream in the cancel cancelAll withdraw withdrawAll withdrawAllTo withdrawTo functions

andreivladbrg · 2022-08-15T12:41:29Z

Is worth mentioning that the changes will increase the gas cost for the create function.

Before: https://imgur.com/a/reBfXXR
After: https://imgur.com/a/fooGyFr

remappings.txt

src/SablierV2Linear.sol

src/SablierV2Pro.sol

src/SablierV2.sol

src/SablierV2Pro.sol

src/SablierV2Linear.sol

src/SablierV2Pro.sol

src/SablierV2.sol

PaulRBerg · 2022-10-22T14:24:10Z

Great work! I have left multiple specific bits of feedback to your code above.

Separately, this is my general feedback:

Common Inheritance

You should have inherited from IERC721 in the interface ISablierV2. This is due to two reasons:

All SablierV2 contracts are expected to be NFTs.
This is the practice that we've been following so far, to aggregate all common interfaces in ISablierV2.

Unfortunately, it doesn't look like Solmate offers any interface for ERC-721. To achieve this, I had to switch over to OpenZeppelin.

Recipient

I agree that it would be useful to keep the getRecipient function and keep referring to the recipient of the stream as a recipient in the code as well, not just in speach and marketing materials. However, I have opened a discussion about this, in case we change our minds later: #141

src/SablierV2Linear.sol

test/unit/sablier-v2-pro/create-with-duration/createWithDuration.tree

test/unit/sablier-v2-linear/withdraw/withdraw.tree

test/unit/sablier-v2-linear/withdraw/withdraw.t.sol

test/unit/sablier-v2-linear/withdraw/withdraw.tree

test/unit/sablier-v2-linear/withdraw/withdraw.t.sol

test/unit/sablier-v2-linear/withdraw/withdraw.tree

test/unit/sablier-v2-linear/withdraw/withdraw.t.sol

test/unit/sablier-v2-linear/withdraw-to/withdrawTo.tree

test/unit/sablier-v2-linear/withdraw/withdraw.t.sol

test/unit/sablier-v2-linear/withdraw-to/withdrawTo.t.sol

test/unit/sablier-v2-linear/cancel/cancel.tree

test/unit/sablier-v2-linear/cancel/cancel.t.sol

src/SablierV2Pro.sol

test/unit/sablier-v2-linear/cancel-all/cancelAll.tree

PaulRBerg · 2022-10-23T18:03:24Z

I have just went through all tests and they generally looked good to me, but there several wordings that needed improvements, which I did in my latest commit.

There are two things left to do in this PR:

Burn the NFT in the cancel function
Decide on whether we should keep using OpenZeppelin's isApprovedOrOwner implementation, or switch to our own.

PaulRBerg · 2022-10-23T19:34:11Z

As discussed privately, we can keep using OZ's _isApprovedOrOwner function for the time being, and decide whether it would be worth it switch to our own implementation later on (to avoid checking for the owner).

What we certainly don't want is to keep isApprovedOrOwner in the ISablierV2. The ISablierV2 interface should be concerned with user-facing functions exclusively.

src/SablierV2.sol

andreivladbrg · 2022-10-24T11:50:55Z

Given that usually the functions which return a boolean use the "is" prefix. I suggest we rename the modifier from isAuthorizedForStream to authorizedForStream.

PaulRBerg · 2022-10-24T12:08:44Z

@andreivladbrg let's discuss that separately outside of this PR. For now I would like to keep isAuthorizedForStream.

Why did you use safeMint instead of mint?

andreivladbrg · 2022-10-24T12:15:53Z

@andreivladbrg let's discuss that separately outside of this PR. For now I would like to keep isAuthorizedForStream.

Ok.

Why did you use safeMint instead of mint?

I followed the openzeppelin recommendation.

Just noticed that gas cost increased by 711 using _safeMint. Should we switch to _mint again?

feat: inherit "ERC721" in "SablierV2Linear" feat: inherit "ERC721" in "SablierV2Pro" feat: mint NFT in the "_create" function feat: add "isApprovedOrOwner" function feat: change the modifier to "isAuthorizedForStream" feat: add "tokenURI" function perf: remove the recipient address from the "Stream" struct refactor: swap the order of the checks in the "withdrawTo" function test: change the struct recipient with the "users.recipient" test: make recipient the default caller in the "cancel" function test: order correctly the branches in the "cancel" tree test: order correctly the branches in the "cancelAll" tree test: order correctly the branches in the "withdraw" tree test: when the caller is an approved third party in the "cancel" function test: when the caller is an approved third party in the "cancelAll" funtion test: when the caller is an approved third party in the "withdraw" function test: when the caller is an approved third party in the "withdrawAll" function test: when the caller is an approved third party in the "withdrawAllTo" function test: when the caller is an approved third party in the "withdtawTo" function test: when the recipient is no longer the owner of the stream in the "cancel" function test: when the recipient is no longer the owner of the stream in the "withdraw" function test: when the recipient is no longer the owner of the streams in the "cancelAll" function test: when the recipient is no longer the owner of the streams in the "withdrawAll" function test: when the recipient is no longer the owner of the streams in the "withdrawAllTo" function test: when the recipient is no longer the owner of the stream in the "withrawTo" function test: remove the local "withdrawAmount" variable from the test functions test: change the new owner of the streams to "eve" when using "safeTransferFrom" function test: "isApprovedOrOwner" function in the "SablierV2Linear" contract test: "isApprovedOrOwner" function in the "SablierV2Pro" contract test: correct comment about return value for the "isCancelable" function

build: remove "solmate" build: install "openzeppelin-contracts" lib build: upgrade to latest "prb-math" lib chore: add ".solhintignore" file chore: improve wording in comments docs: improve wording in NatSpec refactor: change the ERC-721 name refactor: name return args in "isApprovedOrOwner" and "tokenURI" refactor: refer to "to" as "recipient" refactor: use OpenZeppelin's ERC721 implementation refactor: use "_isApprovedOrOwner" from OpenZeppelin test: add dummy function implementations in "AbstractSablierV2" test: delete "isApprovedOrOwner" tests

build: upgrade to "release-v4.8" of "openzeppelin-contracts"

test: add new "operator" user test: add new testing branches for un/authorized test: delete superfluous warps in "withdraw" tests test: move assertions for recipients at the bottom test: refer to approved "third-party" as "operator" test: rename "withdrawAmountZero" to "withdrawAmount" test: refactor "CallerRecipient" to "OriginalRecipient" test: use the new operator user in the "approved operator" tests

chore: add visibility keyword in function separators refactor: inherit from ERC721 last test: simplify AbstractSablierV2 contract

chore: remove unnecessary imports

test: update tests to check for burned NFT

* feat: ERC721 build: install "openzeppelin-contracts" lib feat: inherit "ERC721" in "SablierV2Linear" feat: inherit "ERC721" in "SablierV2Pro" feat: mint NFT in the "_create" function feat: add "isApprovedOrOwner" function feat: change the modifier to "isAuthorizedForStream" feat: add "tokenURI" function perf: remove the recipient address from the "Stream" struct refactor: swap the order of the checks in the "withdrawTo" function test: change the struct recipient with the "users.recipient" test: make recipient the default caller in the "cancel" function test: order correctly the branches in the "cancel" tree test: order correctly the branches in the "cancelAll" tree test: order correctly the branches in the "withdraw" tree test: when the caller is an approved third party in the "cancel" function test: when the caller is an approved third party in the "cancelAll" funtion test: when the caller is an approved third party in the "withdraw" function test: when the caller is an approved third party in the "withdrawAll" function test: when the caller is an approved third party in the "withdrawAllTo" function test: when the caller is an approved third party in the "withdtawTo" function test: when the recipient is no longer the owner of the stream in the "cancel" function test: when the recipient is no longer the owner of the stream in the "withdraw" function test: when the recipient is no longer the owner of the streams in the "cancelAll" function test: when the recipient is no longer the owner of the streams in the "withdrawAll" function test: when the recipient is no longer the owner of the streams in the "withdrawAllTo" function test: when the recipient is no longer the owner of the stream in the "withrawTo" function test: remove the local "withdrawAmount" variable from the test functions test: change the new owner of the streams to "eve" when using "safeTransferFrom" function test: "isApprovedOrOwner" function in the "SablierV2Linear" contract test: "isApprovedOrOwner" function in the "SablierV2Pro" contract test: correct comment about return value for the "isCancelable" function * refactor: inherit from "IERC721" in "ISablierV2" build: upgrade to latest "prb-math" lib chore: add ".solhintignore" file chore: improve wording in comments docs: improve wording in NatSpec refactor: change the ERC-721 name refactor: name return args in "isApprovedOrOwner" and "tokenURI" refactor: refer to "to" as "recipient" refactor: use OpenZeppelin's ERC721 implementation refactor: use "_isApprovedOrOwner" from OpenZeppelin test: add dummy function implementations in "AbstractSablierV2" test: delete "isApprovedOrOwner" tests * fix: use "_ownerOf" ERC721 function build: upgrade to "release-v4.8" of "openzeppelin-contracts" * test: improve wording in testing trees test: add new "operator" user test: add new testing branches for un/authorized test: delete superfluous warps in "withdraw" tests test: move assertions for recipients at the bottom test: refer to approved "third-party" as "operator" test: rename "withdrawAmountZero" to "withdrawAmount" test: refactor "CallerRecipient" to "OriginalRecipient" test: use the new operator user in the "approved operator" tests * refactor: isApprovedOrOwner chore: add visibility keyword in function separators refactor: inherit from ERC721 last test: simplify AbstractSablierV2 contract * chore: remove unnecessary imports * style: order imports chore: remove unnecessary imports * refactor: burn NFT when stream is ended test: update tests to check for burned NFT * refactor: use "safeMint" in the "create" function * test: correct explanatory comment Co-authored-by: Paul Razvan Berg <[email protected]>

andreivladbrg requested a review from PaulRBerg August 15, 2022 12:01

andreivladbrg added dependencies labels Aug 15, 2022

This was referenced Aug 15, 2022

Implement ERC721 standard in both Linear and Pro contracts #124

Closed

Implement ERC721 standard in both Linear and Pro contracts #130

Closed

andreivladbrg force-pushed the andrei/feat-erc-721 branch 2 times, most recently from 45e1a35 to 2a4e756 Compare August 15, 2022 12:36

andreivladbrg force-pushed the andrei/feat-erc-721 branch from 2a4e756 to 3003b34 Compare August 15, 2022 13:48

andreivladbrg mentioned this pull request Aug 25, 2022

Change the type for the time variables #132

Closed

4 tasks

PaulRBerg mentioned this pull request Oct 22, 2022

Figure out a way to get rid of the "AbstractSablierV2" testing contract #139

Closed

andreivladbrg mentioned this pull request Oct 22, 2022

Add libraries for the custom errors, data types, events and validations, optimize pro contract, rename testing contracts #140

Closed

8 tasks

PaulRBerg requested changes Oct 22, 2022

View reviewed changes

PaulRBerg reviewed Oct 22, 2022

View reviewed changes

src/SablierV2Linear.sol Outdated Show resolved Hide resolved