Convert to upgradeable and cloneable contracts #43

jimthematrix · 2024-08-20T18:38:02Z

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 or false 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.

Signed-off-by: Jim Zhang <[email protected]>

codecov · 2024-08-20T18:47:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.23%. Comparing base (dfecf4d) to head (be9e994).
Report is 6 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

Chengxuan

@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.

Chengxuan · 2024-08-21T05:23:18Z

go-sdk/internal/sparse-merkle-tree/storage/sql_test.go

+	dbfile, err := os.CreateTemp("", "gorm.db")
+	assert.NoError(t, err)
+	defer func() {
+		os.Remove(dbfile.Name())


Suggested change

os.Remove(dbfile.Name())

err := os.Remove(dbfile.Name())

assert.NoError(t, err)

don't think we should fail the test if cleaning up the temp folder fails

Chengxuan · 2024-08-21T05:23:30Z

go-sdk/internal/sparse-merkle-tree/storage/sql_test.go

+	dbfile, err := os.CreateTemp("", "gorm.db")
+	assert.NoError(t, err)
+	defer func() {
+		os.Remove(dbfile.Name())


Suggested change

os.Remove(dbfile.Name())

err := os.Remove(dbfile.Name())

assert.NoError(t, err)

Chengxuan · 2024-08-21T05:23:37Z

go-sdk/internal/sparse-merkle-tree/storage/sql_test.go

+	dbfile, err := os.CreateTemp("", "gorm.db")
+	assert.NoError(t, err)
+	defer func() {
+		os.Remove(dbfile.Name())


Suggested change

os.Remove(dbfile.Name())

err := os.Remove(dbfile.Name())

assert.NoError(t, err)```

Chengxuan · 2024-08-21T05:23:52Z

go-sdk/internal/sparse-merkle-tree/storage/sql_test.go

+	dbfile, err := os.CreateTemp("", "gorm.db")
+	assert.NoError(t, err)
+	defer func() {
+		os.Remove(dbfile.Name())


Suggested change

os.Remove(dbfile.Name())

err := os.Remove(dbfile.Name())

assert.NoError(t, err)

Chengxuan · 2024-08-21T05:31:07Z

solidity/contracts/factory.sol

+            authority,
+            _verifier
+        );
+        emit ZetoTokenDeployed(instance);


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.

Chengxuan · 2024-08-21T06:13:25Z

solidity/hardhat.config.ts

+        "7bc522e9ba27f118ad4157771bec290f59ffffe45ee66bb81f137043150bd001",
+        "7bc522e9ba27f118ad4157771bec290f59ffffe45ee66bb81f137043150bd002",
+        "7bc522e9ba27f118ad4157771bec290f59ffffe45ee66bb81f137043150bd003",
+        "7bc522e9ba27f118ad4157771bec290f59ffffe45ee66bb81f137043150bd004",
+        "7bc522e9ba27f118ad4157771bec290f59ffffe45ee66bb81f137043150bd005",


Even though these keys are test data, it's better to not commit them to avoid false security scan alerts.

Chengxuan · 2024-08-21T06:18:02Z

solidity/test/lib/deploy.ts

@@ -0,0 +1,53 @@
+// set this to turn off paramters checking in the deployment scripts
+process.env.TEST_DEPLOY_SCRIPTS = 'true';


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.

Chengxuan · 2024-08-21T06:33:23Z

solidity/contracts/lib/interfaces/zeto_fungible.sol

+import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+
+interface IZetoFungible {
+    function setERC20(IERC20 _erc20) external;


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.

Chengxuan · 2024-08-21T06:33:36Z

solidity/contracts/factory.sol

+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";


not being used

Chengxuan · 2024-08-21T06:35:24Z

solidity/test/lib/deploy.ts

+  }
+
+  if (process.env.USE_FACTORY !== 'true') {
+    // setup via the deployment scripts


adding some console.logs here would be helpful to highlight how the contract was deployed when people look at test run logs

Chengxuan · 2024-08-21T06:57:05Z

@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 mint function will be broken in such case, but I'm not sure about the benefit of allowing such state.

Signed-off-by: Jim Zhang <[email protected]>

jimthematrix · 2024-08-21T15:37:02Z

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 Ownable interface as needed.

Can you give more details on the scenario where this would be broken?

jimthematrix · 2024-08-21T15:58:19Z

I think the naming of authority needs some clarification. Reading the logic initialOwner seems to be a better name for now.

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 Ownable parent contract anyway, which calls it initialOwner

Signed-off-by: Jim Zhang <[email protected]>

Chengxuan · 2024-08-21T16:48:38Z

@jimthematrix I think you've answered my question. Thanks.

Can you give more details on the scenario where this would be broken?

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?

Chengxuan

LGTM. I'm happy to work on the outstanding proposal for transferOwnership as a follow-on PR if @jimthematrix agrees on it.

Convert to upgradeable contracts

9d40e02

Signed-off-by: Jim Zhang <[email protected]>

Merge branch 'libs' into upgradeable

87b0404

Chengxuan reviewed Aug 21, 2024

View reviewed changes

jimthematrix added 2 commits August 21, 2024 10:39

Merge branch 'main' into upgradeable

516267b

Addressing review comments

d7c4ffe

Signed-off-by: Jim Zhang <[email protected]>

Change variable naming of to

be9e994

Signed-off-by: Jim Zhang <[email protected]>

Chengxuan self-requested a review August 21, 2024 17:01

Chengxuan approved these changes Aug 21, 2024

View reviewed changes

jimthematrix merged commit 3b7a354 into main Aug 22, 2024
7 checks passed

jimthematrix deleted the upgradeable branch August 22, 2024 03:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert to upgradeable and cloneable contracts #43

Convert to upgradeable and cloneable contracts #43

jimthematrix commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 20, 2024 •

edited

Loading

Chengxuan left a comment •

edited

Loading

Chengxuan Aug 21, 2024

jimthematrix Aug 21, 2024

Chengxuan Aug 21, 2024

Chengxuan Aug 21, 2024

Chengxuan Aug 21, 2024

Chengxuan Aug 21, 2024

Chengxuan Aug 21, 2024

Chengxuan Aug 21, 2024

Chengxuan Aug 21, 2024 •

edited

Loading

Chengxuan Aug 21, 2024

Chengxuan Aug 21, 2024 •

edited

Loading

Chengxuan commented Aug 21, 2024

jimthematrix commented Aug 21, 2024

jimthematrix commented Aug 21, 2024

Chengxuan commented Aug 21, 2024 •

edited

Loading

Chengxuan left a comment

	os.Remove(dbfile.Name())
	err := os.Remove(dbfile.Name())
	assert.NoError(t, err)

	os.Remove(dbfile.Name())
	err := os.Remove(dbfile.Name())
	assert.NoError(t, err)```

		@@ -0,0 +1,53 @@
		// set this to turn off paramters checking in the deployment scripts
		process.env.TEST_DEPLOY_SCRIPTS = 'true';

Convert to upgradeable and cloneable contracts #43

Convert to upgradeable and cloneable contracts #43

Conversation

jimthematrix commented Aug 20, 2024 • edited Loading

codecov bot commented Aug 20, 2024 • edited Loading

Codecov Report

Chengxuan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chengxuan Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chengxuan Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Chengxuan commented Aug 21, 2024

jimthematrix commented Aug 21, 2024

jimthematrix commented Aug 21, 2024

Chengxuan commented Aug 21, 2024 • edited Loading

Chengxuan left a comment

Choose a reason for hiding this comment

jimthematrix commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 20, 2024 •

edited

Loading

Chengxuan left a comment •

edited

Loading

Chengxuan Aug 21, 2024 •

edited

Loading

Chengxuan Aug 21, 2024 •

edited

Loading

Chengxuan commented Aug 21, 2024 •

edited

Loading