Skip to content
This repository has been archived by the owner on Dec 3, 2024. It is now read-only.

Upgrading to OpenZeppelin v5 and Solidity 0.8.20 #196

Merged
merged 20 commits into from
Jul 18, 2024

Conversation

minghinmatthewlam
Copy link

Why this should be merged

Fixes #188

How this works

  • Adds OZ contracts and contracts-upgradeable to lib instead of pointing the remapping to teleporter's lib
  • SafeERC20.forceApprove instead of safeApprove
  • OZ v5 removes IERC20Upgradeable and instead upgradeable ERC20s still implement IERC20
  • WrappedNativeToken does not need to be upgradeable.
  • When the internal _spendAllowance is called, previous OZ v4 would emit Approval, but in v5 by default it will not emit the event, so removed that emit from unit tests.
  • _burn is moved above _handleFees for token remotes, comparable to how token home contracts deposit then handle fees
  • Unit tests account for OZ v5 custom errors
  • updates to corresponding teleporter upgraded versions commit

How this was tested

CI

How is this documented

update comments/readme

Copy link
Contributor

@iansuvak iansuvak left a comment

Choose a reason for hiding this comment

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

LGTM other than the typo, but looks like the e2e tests are still failing

tests/utils/utils.go Outdated Show resolved Hide resolved
tests/utils/utils.go Outdated Show resolved Hide resolved
Copy link
Contributor

@bernard-avalabs bernard-avalabs left a comment

Choose a reason for hiding this comment

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

Generally makes sense to me. Just pointed a comment that might need to be updated.

contracts/src/TokenHome/ERC20TokenHome.sol Outdated Show resolved Hide resolved
@minghinmatthewlam minghinmatthewlam merged commit f05f940 into namespace-storage Jul 18, 2024
6 of 9 checks passed
@minghinmatthewlam minghinmatthewlam deleted the version-updates branch July 18, 2024 18:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants