Add conditions around solidity failure cases for hardhat network only #46

jimthematrix · 2024-08-23T19:41:28Z

Several updates related to testing against a non-hardhat network, as well as public chains.

group together the failure tests and add a condition to skip when the target network is not hardhat. this is because the detailed error messages are only returned by hardhat, but not other types of EVM networks (would just be the generic "EVM transaction reverted")
change the balance checking after deposit and withdraw from equal to gte. This doesn't impact runs against the hardhat network. Ignition only does the caching of the deployed contracts, to save on gas apparently, when deploying against a non-hardhat contract. So the change to check on greater than or equal to is equivalent to equal to when running against the hardhat network, but will avoid test failures when running against non-hardhat network
specifically for public networks, increasing the timeout of the before() method to accommodate the longer deployment time

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

codecov · 2024-08-23T19:44:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.11%. Comparing base (3b7a354) to head (fcf39b6).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
+ Coverage   73.23%   78.11%   +4.88%     
==========================================
  Files          12       13       +1     
  Lines         538      553      +15     
==========================================
+ Hits          394      432      +38     
+ Misses        103       83      -20     
+ Partials       41       38       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Chengxuan

@jimthematrix I like the intention of reusing existing tests to drive public network testing.

However, I feel we should preserve the same level of checks and behaviors for running local tests where possible. I added a few comments from that angle.

As the testing against a non-hardhat network is not part of the automated pipeline, preventing it from deteriorating is another problem to consider, but might not be an immediate concern.

solidity/test/zeto_anon.ts

Chengxuan · 2024-08-27T06:24:50Z

solidity/test/zeto_anon.ts

@@ -64,7 +61,7 @@ describe("Zeto based fungible token with anonymity without encryption or nullifi
    const tx = await erc20.connect(deployer).mint(Alice.ethAddress, 100);
    await tx.wait();
    const balance = await erc20.balanceOf(Alice.ethAddress);
-    expect(balance).to.equal(100);
+    expect(balance).to.be.gte(100);


I feel we shouldn't sacrifice the scope of assertions in the tests when running against a hardhat network.

This doesn't impact runs against the hardhat network. Ignition is pretty smart, it only does the caching of the deployed contracts, to save on gas apparently, when deploying against a non-hardhat contract. So the change to check on greater than or equal to is equivalent to equal to when running against the hardhat network.

Here's the abridged output from runs against the hardhat network, you can see the ERC20 contract is re-deployed for every test suite:

Zeto based fungible token with anonymity without encryption or nullifier Deploying as upgradeable contracts ZetoToken deployed: 0x5FC8d32690cc91D4c39d9d3abcBD16989F875707 ERC20 deployed: 0x5FbDB2315678afecb367f032d93F642f64180aa3 ... Zeto based fungible token with anonymity and encryption Deploying as upgradeable contracts ZetoToken deployed: 0x9A9f2CCfdE556A7E9Ff0848998Aa4a0CFD8863AE ERC20 deployed: 0xA51c1fc2f0D1a1b8494Ed1FE312d7C3a78Ed91C0 ... Zeto based fungible token with anonymity using nullifiers and encryption Deploying as upgradeable contracts ZetoToken deployed: 0x84eA74d481Ee0A5332c457a4d796187F6Ba67fEB ERC20 deployed: 0xa85233C63b9Ee964Add6F2cffe00Fd84eb32338f

On the other hand, making this change avoids failures in testing against a non-hardhat network, because the same ERC20 contract is used for all test suites, due to ignition layer caching.

So if the smart contract accidentally doubled the amount in the transfer logic, this check will throw an error because it knows it's running in Hardhat mode, therefore, gte need to be switched to equal?

not sure I understand the question @Chengxuan ?

I think the best way to address the concern of missing implementation errors like you pointed out before, without sacrificing public chain testability (where the ERC20 contract is re-used across the test suites due to ignition caching), is querying for the starting balance and ending balance around the Zeto operation. This way we can change back the check to equal.

EnriqueL8

Agree with the concerns from @Chengxuan on making sure we have a reliable set of tests against a non-hardhat setup and we don't hide issues by simply extending the timeouts.

Could you also provide a description in this PR?

EnriqueL8 · 2024-08-27T10:27:18Z

solidity/scripts/deploy_cloneable.ts

-  const opts = {
-    kind: 'uups',
-    initializer: 'initialize',
-    unsafeAllow: ['delegatecall']
-  };


I thought we needed this for upgradable contracts?

It's accurate that this options object is needed for upgradeable contracts, but here we are deploying the implementation contract that can then be cloned, so no need for these parameters.

What we could do is, demonstrate how the cloned instance can then be turned into an upgradeable contract. But that's making the sample too complex (first deploy the implementation, then deploy the clone factory, then create the clone, and then make it into an upgradeable).

sounds good

solidity/test/zeto_anon.ts

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

Chengxuan

Thanks for addressing the comments. @jimthematrix

Add conditions around solidity failure cases for hardhat network only

dfbd1d0

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

jimthematrix added 4 commits August 23, 2024 17:26

Minor updates to remove debugging logs

9158c94

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

Add setERC20() calls in cloneable contract deploy scripts

8d0c25f

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

Only call setERC20() for fungible tokens

b3b8e80

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

Add more unit test for go-sdk

fcf39b6

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

Chengxuan reviewed Aug 27, 2024

View reviewed changes

Chengxuan requested review from awrichar and EnriqueL8 August 27, 2024 06:33

EnriqueL8 reviewed Aug 27, 2024

View reviewed changes

jimthematrix force-pushed the public-chains branch from ff51688 to f57d353 Compare August 27, 2024 14:00

Address review comments

f2bf618

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

jimthematrix force-pushed the public-chains branch from f57d353 to f2bf618 Compare August 27, 2024 14:04

jimthematrix added 2 commits August 27, 2024 10:06

Remove .openzeppelin metadata

6fb652c

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

calculate balance differences

f8b7cc9

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

Chengxuan approved these changes Aug 27, 2024

View reviewed changes

Chengxuan merged commit f9b4a17 into main Aug 28, 2024
5 checks passed

Chengxuan deleted the public-chains branch August 28, 2024 04:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conditions around solidity failure cases for hardhat network only #46

Add conditions around solidity failure cases for hardhat network only #46

jimthematrix commented Aug 23, 2024 •

edited

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

Chengxuan left a comment

Chengxuan Aug 27, 2024

jimthematrix Aug 27, 2024 •

edited

Loading

Chengxuan Aug 27, 2024

jimthematrix Aug 27, 2024

jimthematrix Aug 27, 2024

EnriqueL8 left a comment

EnriqueL8 Aug 27, 2024

jimthematrix Aug 27, 2024 •

edited

Loading

EnriqueL8 Aug 27, 2024

Chengxuan left a comment

Add conditions around solidity failure cases for hardhat network only #46

Add conditions around solidity failure cases for hardhat network only #46

Conversation

jimthematrix commented Aug 23, 2024 • edited Loading

codecov bot commented Aug 23, 2024 • edited Loading

Codecov Report

Chengxuan left a comment

Choose a reason for hiding this comment

Chengxuan Aug 27, 2024

Choose a reason for hiding this comment

jimthematrix Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Chengxuan Aug 27, 2024

Choose a reason for hiding this comment

jimthematrix Aug 27, 2024

Choose a reason for hiding this comment

jimthematrix Aug 27, 2024

Choose a reason for hiding this comment

EnriqueL8 left a comment

Choose a reason for hiding this comment

EnriqueL8 Aug 27, 2024

Choose a reason for hiding this comment

jimthematrix Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

EnriqueL8 Aug 27, 2024

Choose a reason for hiding this comment

Chengxuan left a comment

Choose a reason for hiding this comment

jimthematrix commented Aug 23, 2024 •

edited

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

jimthematrix Aug 27, 2024 •

edited

Loading

jimthematrix Aug 27, 2024 •

edited

Loading