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(ptoken): <- remove all ERC777 hooks from that #67

Merged
merged 10 commits into from
Dec 16, 2022

Conversation

gskapka
Copy link
Collaborator

@gskapka gskapka commented Dec 15, 2022

...per title.

NOTE: Hook calls have been commented out in the contract in question, though the hook logic remains.

NOTE: Test w/r/t the tokensReceivedHook have been skipped for now.

Copy link
Collaborator

@allemanfredi allemanfredi left a comment

Choose a reason for hiding this comment

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

LGTM!

btw i would add one test to test the upgrade just to be sure. You could:

  • create a test contract called OldErc777 (or something like this) that still uses the real ERC777Upgradable implementation (import "@openzeppelin/contracts-upgradeable/token/ERC777/ERC777Upgradeable.sol")
  • deploy the pToken contract using OldErc777
  • do something just to write some value in the contract state
  • upgrade the pToken contract using the the new implementation (import "./ERC777Upgradeable.sol")

@gskapka
Copy link
Collaborator Author

gskapka commented Dec 15, 2022

I've done one better because we need a more robust test for the upgrade compatibility since it's so critical.

So if you review the latest additions you'll see that new test.

Basically we use a child process to clone the whole repo to temp dir, checkout master there, flatten the pToken.sol contract, copy that contract into the main repo's ./contracts dir, then finally use ethers' upgrades.validateUpgrade fxn to check we've not broken the storage compatibility.

Having this test here (and runnable in the CI which I've also added) gives me much more confidence going forward since we're likely to be editing this contract more in future.

@gskapka gskapka force-pushed the remove-all-erc777-hooks branch 8 times, most recently from 6158620 to b72cdc8 Compare December 15, 2022 17:44
Copy link
Collaborator

@allemanfredi allemanfredi left a comment

Choose a reason for hiding this comment

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

LGTM!

@gskapka gskapka merged commit a271df9 into master Dec 16, 2022
@gskapka gskapka deleted the remove-all-erc777-hooks branch December 16, 2022 13:02
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.

None yet

2 participants