-
Notifications
You must be signed in to change notification settings - Fork 12
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
Convert to upgradeable and cloneable contracts #43
Conversation
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
==========================================
+ Coverage 71.37% 73.23% +1.85%
==========================================
Files 12 12
Lines 538 538
==========================================
+ Hits 384 394 +10
+ Misses 109 103 -6
+ Partials 45 41 -4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jimthematrix great work. I added some comments as I read through the code. I think the naming of authority
needs some clarification. Reading the logic initialOwner
seems to be a better name for now.
dbfile, err := os.CreateTemp("", "gorm.db") | ||
assert.NoError(t, err) | ||
defer func() { | ||
os.Remove(dbfile.Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.Remove(dbfile.Name()) | |
err := os.Remove(dbfile.Name()) | |
assert.NoError(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't think we should fail the test if cleaning up the temp folder fails
dbfile, err := os.CreateTemp("", "gorm.db") | ||
assert.NoError(t, err) | ||
defer func() { | ||
os.Remove(dbfile.Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.Remove(dbfile.Name()) | |
err := os.Remove(dbfile.Name()) | |
assert.NoError(t, err) |
dbfile, err := os.CreateTemp("", "gorm.db") | ||
assert.NoError(t, err) | ||
defer func() { | ||
os.Remove(dbfile.Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.Remove(dbfile.Name()) | |
err := os.Remove(dbfile.Name()) | |
assert.NoError(t, err)``` |
dbfile, err := os.CreateTemp("", "gorm.db") | ||
assert.NoError(t, err) | ||
defer func() { | ||
os.Remove(dbfile.Name()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.Remove(dbfile.Name()) | |
err := os.Remove(dbfile.Name()) | |
assert.NoError(t, err) |
authority, | ||
_verifier | ||
); | ||
emit ZetoTokenDeployed(instance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels there are other information that could be useful to emit. But they can be added based on the use case in the future.
solidity/hardhat.config.ts
Outdated
"7bc522e9ba27f118ad4157771bec290f59ffffe45ee66bb81f137043150bd001", | ||
"7bc522e9ba27f118ad4157771bec290f59ffffe45ee66bb81f137043150bd002", | ||
"7bc522e9ba27f118ad4157771bec290f59ffffe45ee66bb81f137043150bd003", | ||
"7bc522e9ba27f118ad4157771bec290f59ffffe45ee66bb81f137043150bd004", | ||
"7bc522e9ba27f118ad4157771bec290f59ffffe45ee66bb81f137043150bd005", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though these keys are test data, it's better to not commit them to avoid false security scan alerts.
@@ -0,0 +1,53 @@ | |||
// set this to turn off paramters checking in the deployment scripts | |||
process.env.TEST_DEPLOY_SCRIPTS = 'true'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth adding some documentation about TEST_DEPLOY_SCRIPTS
and ZETO_NAME
, otherwise, the reader will need to parse it out from code reading.
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; | ||
|
||
interface IZetoFungible { | ||
function setERC20(IERC20 _erc20) external; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name of this file doesn't reflect it's mainly for supporting ERC20 contract anchoring, also, the interface is not used by multiple files as the existing abstract contract ZetoFungible
seems to be sufficient for reusability. So it feels like we might not need this separate interface for now.
solidity/contracts/factory.sol
Outdated
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; | ||
import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; | ||
import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; | ||
import {IZetoFungible} from "./lib/interfaces/zeto_fungible.sol"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not being used
} | ||
|
||
if (process.env.USE_FACTORY !== 'true') { | ||
// setup via the deployment scripts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding some console.logs here would be helpful to highlight how the contract was deployed when people look at test run logs
@jimthematrix I might have missed this during my code reading. For ERC20 anchored token, should Zeto owner be updated when the owner of ERC20 token is not upgradeable, I can see a draw back that the Zeto |
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
Both the sample ERC20 and the Zeto tokens have the flexibility to set their authority/owner in the constructor/initializer. Then afterwards they can be updated through the Can you give more details on the scenario where this would be broken? |
That's a good point. While a client might use an authority account to deploy the contracts and set it as the owner, from the contract's point of view, we are really just setting the owner. Plus we are just overriding the default behavior of the |
Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
@jimthematrix I think you've answered my question. Thanks.
The mint function will be broken when the ERC20 that had been set is not OwnableUpgradeable. (Currently there is no code enforcing that) Do you feel ea7941d is something we should have to spell out which types of ERC20 token can be anchored and do the transfer in one call? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'm happy to work on the outstanding proposal for transferOwnership as a follow-on PR if @jimthematrix agrees on it.
Fixes #42
All the token implementation contracts have been converted to be upgradeable as an
UUPSUpgradeable
.In addition, they are also compatible with the
Clones.sol
factory in openzeppelin. This allows any of the Zeto tokens to be cheaply deployed via the factory contract.A factory contract has been provided that utilizes the Clones factory.
Each hardhat test can now be run with
USE_FACTORY=true
orfalse
to deploy the token via the Clones factory or the UUPSUpgradeable. The e2e test github action has been updated to run the tests in both mechanisms.