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

update: upgrade @openzeppelin contract packages to latest 5.0.0 version (#536) #541

Merged

Conversation

quiet-node
Copy link
Member

@quiet-node quiet-node commented Oct 27, 2023

Description:

  • Upgrade @openzeppelin/contracts and @openzeppelin/upgradable-contracts to latest v5.0.0
  • Modified smart contracts utilizing OZ contracts to match with latest changes
  • Fixed warnings
  • Recompiled all contracts
  • In order to bypass the upgrade safe checks from hardhat-updates's .deployProxy() method, contract OZUUPSUpgradeableV4 is added. This OZUUPSUpgradeableV4 is a clone of the UUPSUpgradable contract from OZ @v5.x but with an extra upgradeTo function. This contract is only for unblocking purpose and should be deprecated in the future release when we upgrade to [email protected]

Related issue(s):

Fixes #536

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@quiet-node quiet-node added enhancement New feature or request P2 EVM evm related labels Oct 27, 2023
@quiet-node quiet-node self-assigned this Oct 27, 2023
@quiet-node quiet-node marked this pull request as draft October 27, 2023 19:25
@github-actions
Copy link

github-actions bot commented Oct 27, 2023

Test Results

  15 files  ±0    65 suites   - 1   8m 0s ⏱️ +13s
195 tests  - 3  188 ✔️  - 2  6 💤 ±0  1  - 1 
205 runs   - 3  198 ✔️  - 2  6 💤 ±0  1  - 1 

For more details on these failures, see this check.

Results for commit 04191df. ± Comparison against base commit e0413d3.

This pull request removes 3 tests.
should create a new snapshot and emit a Snapshot event ‑ ERC20ExtensionsMock tests ERC20Snapshot tests should create a new snapshot and emit a Snapshot event
should return the correct balanceOfAt(address, snapshotId) ‑ ERC20ExtensionsMock tests ERC20Snapshot tests should return the correct balanceOfAt(address, snapshotId)
should return the correct totalSupplyAt(snapshotId) ‑ ERC20ExtensionsMock tests ERC20Snapshot tests should return the correct totalSupplyAt(snapshotId)

♻️ This comment has been updated with latest results.

@quiet-node quiet-node force-pushed the 536-Upgrade-@openzeppelin/contracts-npm-package-to-latest-5.x- branch 2 times, most recently from ae3fc1e to f9a2db5 Compare October 27, 2023 22:20
@@ -138,15 +138,6 @@ contract HederaNonFungibleToken is ERC721, Constants {
return super.transferFrom(from, to, tokenId);
}

function safeTransferFrom(address from, address to, uint256 tokenId) public override {
Copy link
Member Author

Choose a reason for hiding this comment

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

notice: This is being removed as OZ/contract v5.0.0 only allows override safeTransferFrom(address,address,uint256,bytes), and safeTransaferFrom(address,address,uint256) will cause error Trying to override non-virtual function.

@quiet-node quiet-node marked this pull request as ready for review November 1, 2023 15:29
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

Great work, blocking until we cut a 0.6 release branch

@stefan-stefanooov
Copy link
Contributor

stefan-stefanooov commented Nov 1, 2023

Nicely done ... I have several branches that I want to rebase on this one. Waiting for the merger...
@Nana-EC what is the plan ? When are we cutting 0.6 ?

As OZ/contract v5.0.0 only allow override safeTransferFrom(address,address,uint256,bytes), safeTransaferFrom(address,address,uint256) will cause error `Trying to override non-virtual function.`

Signed-off-by: Logan Nguyen <[email protected]>
@quiet-node quiet-node force-pushed the 536-Upgrade-@openzeppelin/contracts-npm-package-to-latest-5.x- branch from b5903c3 to 5b5b4bb Compare November 3, 2023 21:49
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

Mostly good.
2 items

@@ -0,0 +1,176 @@
// SPDX-License-Identifier: MIT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// SPDX-License-Identifier: MIT
// SPDX-License-Identifier: Apache-2.0

@@ -14,8 +14,4 @@ contract Main is Base {
function returnSuper() public view virtual returns (string memory) {
return super.classIdentifier();
}

function destroyContract(address recipient) public {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please restore.
We can remove it separately but until Cancun let's maintain the coverage

@@ -8,6 +8,6 @@ OPERATOR_KEY_A= 302e020100300506032b65700422042091132178e72057a1d7528025956fe39b
# ECDSA HEX Encoded Private Key
HEX_PRIVATE_KEY_A= 0x2e1d968b041d84dd120a5860cee60cd83f9374ef527ca86996317ada3d0d03e7
HEX_PRIVATE_KEY_B= 0x45a5a7108a18dd5013cf2d5857a28144beadc9c70b3bdbd914e38df4e804b8d8
RETRY_DELAY=3000 # ms
RETRY_DELAY=4000 # ms
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q:why the increase?

Copy link
Member Author

Choose a reason for hiding this comment

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

The should be able to execute cryptoTransfer for hbar transfer only unit test in the TokenTransferContract Test Suite test suite was consistently failing with as the data doesn't have enough time to reflect the changes so I decided to increase it for 1000 more ms.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand but it seems strange that 1 min (3000 ms * 20) isn't enough.
Now we're making it 80s (4000ms *20) something is up.
What is happening that requires this long of retries?

@quiet-node quiet-node merged commit 271fa2e into main Nov 3, 2023
20 of 21 checks passed
@quiet-node quiet-node deleted the 536-Upgrade-@openzeppelin/contracts-npm-package-to-latest-5.x- branch November 3, 2023 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request EVM evm related P2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OZ 5.x Contract Support] Upgrade @openzeppelin/contracts npm package to latest 5.x
3 participants