Integration tests #22
Conversation
|
Feel free to skip those CastVote tests in the repo for now @KeltonMad. (i.e. mark them |
There was a problem hiding this comment.
looks great! My only concerns have to do with approvals, especially in the new integration test. It seems we have some tests where we're approving a lot of calls for things I'm not so sure we need. More approvals means more TXes and we ought to make sure we minimize them wherever possible.
| @@ -20,10 +20,18 @@ contract LinkableCompTokenAnchorPoseidon2 is LinkableAnchorPoseidon2 { | |||
| uint256 _denomination, | |||
| uint32 _merkleTreeHeight, | |||
| uint32 _chainID, | |||
| IMintableCompToken _token | |||
| TokenWrapper _token | |||
There was a problem hiding this comment.
It's always more optimised to use interfaces rather than importing the token contract directly (due to bytecode size). If you think we should use the TokenWrapper as the base then I would consider renaming IMintableCompToken or creating a new ITokenWrapper that handles this.
There was a problem hiding this comment.
I will create a TokenWrapper Interface!
| proof = res.proof; | ||
| publicSignals = res.publicSignals; | ||
| let vKey = await snarkjs.zKey.exportVerificationKey('test/fixtures/circuit_final.zkey'); | ||
| res = await snarkjs.groth16.verify(vKey, publicSignals, proof); |
There was a problem hiding this comment.
you can remove the native verification from these tests
| publicSignals = res.publicSignals; | ||
| vKey = await snarkjs.zKey.exportVerificationKey('test/fixtures/circuit_final.zkey'); | ||
| res = await snarkjs.groth16.verify(vKey, publicSignals, proof); | ||
| assert.strictEqual(res, true); |
There was a problem hiding this comment.
these can be removed from integration tests.
|
Also @KeltonMad I noticed some stylistic changes with comments (capitals vs not). I'd recommend being consistent in the comments at all times. It can lead us to a well-defined process if we do, and otherwise break one down if not. |
Co-authored-by: drewstone <drewstone329@gmail.com>
Co-authored-by: drewstone <drewstone329@gmail.com>
| // approve tokenwrapper to transfer destTokens from user2 | ||
| await destToken.approve(destWrapperToken.address, initialTokenMintAmount, {from: user2}); | ||
| // increase allowance for user1 to burn | ||
| await destWrapperToken.approve(DestChainLinkableAnchorInstance.address, initialTokenMintAmount, {from: user1}); |
There was a problem hiding this comment.
Can you explain this one more? Increase allowance for a burn? If this is removed does the test fail?
There was a problem hiding this comment.
Yes the test fails if this is removed. Burning requires an allowance similar to transferFrom. I am working on ways to optimize all these approvals for deposit-and-wrap and something similar with withdrawing unwrapping and burning.
|
Ping me whenever this is ready again @KeltonMad |
Everything passes except for GovBravo CastVote tests which were failing already.