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.

Sign up for GitHub

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

Jump to bottom

Upgrade to OpenZeppelin V5 #806

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Upgrade to OpenZeppelin V5 #806

merged 2 commits into from
Feb 5, 2024

Conversation

andreivladbrg
Copy link
Member

Closes #702.

Notes about PR:

  • OpenZeppelin V5 removes _after and _before hooks, and implements a _update one
  • some of the functions and parameters have been renamed
  • since ERC20 and ERC721 are now abtract contracts, mocks were required. i know that forge-std offers an implementation, but they don't implemented the OpenZeppelin version (which we need in our code for the hooks called) and it would not be consistent. also, we would have needed to upgrade to the a newer version, which would trigger other incompatibilities. thus, it is better to keep the mocks in our code base.
  • OpenZeppelin uses custom errors now
Snip from release image

@andreivladbrg
build: update openzeppelin to 5.0.0
bde699b 
refactor: remove _after and _before hooks and implement _update function
refactor: use newly added _requireOwned function
refactor: use the new named parameter in ERC20 functions
test: add mocks for ERC20 and ERC721
test: use openeppelin's custom errors in cheatcode
chore: provide a more precise explanation for _update
chore: define "_update" alphabetically
docs: document "_update" function
test: check IERC20Errors custom error revert
@andreivladbrg andreivladbrg mentioned this pull request Feb 2, 2024
Merged
smol-ninja
smol-ninja requested changes Feb 5, 2024
View reviewed changes
src/abstracts/SablierV2Lockup.sol Show resolved Hide resolved
src/abstracts/SablierV2Lockup.sol Outdated Show resolved Hide resolved
src/abstracts/SablierV2Lockup.sol Outdated Show resolved Hide resolved
@andreivladbrg
perf: optimize getRecipient function
7861ad9 
docs: use known terminology in _update natspec
@andreivladbrg andreivladbrg merged commit a55ced2 into staging Feb 5, 2024
7 of 8 checks passed
@smol-ninja smol-ninja deleted the build/open-zeppelin-v5 branch February 5, 2024 12:23
@PaulRBerg

This comment was marked as outdated.

Sign in to view
@andreivladbrg

This comment was marked as resolved.

Sign in to view
@PaulRBerg

This comment was marked as resolved.

Sign in to view
andreivladbrg added a commit that referenced this pull request Feb 12, 2024
@andreivladbrg
Upgrade to OpenZeppelin V5 (#806)
45980e7 
* build: update openzeppelin to 5.0.0

refactor: remove _after and _before hooks and implement _update function
refactor: use newly added _requireOwned function
refactor: use the new named parameter in ERC20 functions
test: add mocks for ERC20 and ERC721
test: use openeppelin's custom errors in cheatcode
chore: provide a more precise explanation for _update
chore: define "_update" alphabetically
docs: document "_update" function
test: check IERC20Errors custom error revert

* perf: optimize getRecipient function

docs: use known terminology in _update natspec
andreivladbrg added a commit that referenced this pull request Feb 15, 2024
@andreivladbrg
Upgrade to OpenZeppelin V5 (#806)
075c205 
* build: update openzeppelin to 5.0.0

refactor: remove _after and _before hooks and implement _update function
refactor: use newly added _requireOwned function
refactor: use the new named parameter in ERC20 functions
test: add mocks for ERC20 and ERC721
test: use openeppelin's custom errors in cheatcode
chore: provide a more precise explanation for _update
chore: define "_update" alphabetically
docs: document "_update" function
test: check IERC20Errors custom error revert

* perf: optimize getRecipient function

docs: use known terminology in _update natspec
andreivladbrg added a commit that referenced this pull request Feb 17, 2024
@andreivladbrg
Upgrade to OpenZeppelin V5 (#806)
110211f 
* build: update openzeppelin to 5.0.0

refactor: remove _after and _before hooks and implement _update function
refactor: use newly added _requireOwned function
refactor: use the new named parameter in ERC20 functions
test: add mocks for ERC20 and ERC721
test: use openeppelin's custom errors in cheatcode
chore: provide a more precise explanation for _update
chore: define "_update" alphabetically
docs: document "_update" function
test: check IERC20Errors custom error revert

* perf: optimize getRecipient function

docs: use known terminology in _update natspec
PaulRBerg
PaulRBerg reviewed Apr 4, 2024
View reviewed changes
script/Init.s.sol
@@ -14,7 +14,7 @@ import { Broker, LockupDynamic, LockupLinear } from "../src/types/DataTypes.sol"
import { BaseScript } from "./Base.s.sol";

interface IERC20Mint {
function mint(address beneficiary, uint256 amount) external;
function mint(address beneficiary, uint256 value) external;
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change the amount to value here?

Copy link
Member Author

Choose a reason for hiding this comment

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

because OpenZeppelin does not say amount anymore but value for all function parameter names

so, i wanted to follow their approach

@PaulRBerg
Copy link
Member

Just found a moment to review this PR. Good job, overall.

they don't implemented the OpenZeppelin version

What do you mean by this? We've upgraded Forge Std in the meantime, so maybe we can switch to their mocks now?

@PaulRBerg
Copy link
Member

@andreivladbrg can we please get an answer from you here

@PaulRBerg PaulRBerg mentioned this pull request Apr 9, 2024
Merged
@andreivladbrg
Copy link
Member Author

andreivladbrg commented Apr 9, 2024 •
edited
Loading

Sorry for not responding to this earlier. I did not receive an email notification for the first message.

The mock contracts in forge-std are replicas of solmate's ERC20 & ERC721, the reason being that they are very minimalist.

IIRC, since we implement the ERC721._update hook, which is expected to be called in multiple functions (transfer, mint, burn, etc.) in OpenZeppelin version, solmate does not have this implementation. Therefore, a lot of the tests would fail because of this. (e.g. isTransferable)

Later edit: I was wrong, we can remove the ERC721Mock 188e244

For the ERC20, I guess we could switch to the forge-std version, but we must test it to see if it works.

Does this answer your question?

@andreivladbrg
Copy link
Member Author

andreivladbrg commented Apr 9, 2024 •
edited
Loading

Regarding the ERC20, the solmate version does not inherit an interface IERC20, and we would need to refactor our tests to take as parameter an address instead of IERC20. Also, the functions have different named params, so it would be inconsistency between src and test.

Do you consider that it would be needed to remove the ERC20 mock? @PaulRBerg

I personally don't think we should make this change atm.

@PaulRBerg
Copy link
Member

Does this answer your question?

Yes, thank you for answering. Let's proceed with using their ERC-721 mock.

Do you consider that it would be needed to remove the ERC20 mock? @PaulRBerg

No. Let's keep using our mock. It would be nice if OpenZeppelin offered a mock for this, though ..

@PaulRBerg PaulRBerg mentioned this pull request May 16, 2024
Merged
andreivladbrg added a commit that referenced this pull request Jul 3, 2024
@andreivladbrg
Upgrade to OpenZeppelin V5 (#806)
24f32da 
* build: update openzeppelin to 5.0.0

refactor: remove _after and _before hooks and implement _update function
refactor: use newly added _requireOwned function
refactor: use the new named parameter in ERC20 functions
test: add mocks for ERC20 and ERC721
test: use openeppelin's custom errors in cheatcode
chore: provide a more precise explanation for _update
chore: define "_update" alphabetically
docs: document "_update" function
test: check IERC20Errors custom error revert

* perf: optimize getRecipient function

docs: use known terminology in _update natspec
andreivladbrg added a commit that referenced this pull request Jul 3, 2024
@andreivladbrg
Upgrade to OpenZeppelin V5 (#806)
c916b2f 
* build: update openzeppelin to 5.0.0

refactor: remove _after and _before hooks and implement _update function
refactor: use newly added _requireOwned function
refactor: use the new named parameter in ERC20 functions
test: add mocks for ERC20 and ERC721
test: use openeppelin's custom errors in cheatcode
chore: provide a more precise explanation for _update
chore: define "_update" alphabetically
docs: document "_update" function
test: check IERC20Errors custom error revert

* perf: optimize getRecipient function

docs: use known terminology in _update natspec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaulRBerg PaulRBerg PaulRBerg left review comments

@smol-ninja smol-ninja smol-ninja approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andreivladbrg @PaulRBerg @smol-ninja