ZK-382: Implement deposit for token shortlist #97
Conversation
|
kroist
requested review from
fbielejec,
DamianStraszak and
guspiel
and removed request for
DamianStraszak
kroist
changed the title
contracts/ShielderV0_1_0.sol
Outdated
| * @custom:oz-upgrades-unsafe-allow external-library-linking | ||
| */ | ||
| // solhint-disable-next-line contract-name-camelcase | ||
| contract ShielderV0_1_0 is |
There was a problem hiding this comment.
The approach in this PR is to keep existing code (shielder-circuits package, integration-tests) pointing at the current mainnet release implementation, and to start adding new code next to it.
An alternative approach would be to modify Shielder.sol etc. in place and add the mainnet release version next to it (in ShielderV_0_0_1.sol and other files if necessary).
The latter seems better to me. V_0_0_1 is already tested, we don't need to run any tests for it. But it seems cleaner to work on the V_0_1_0 implementation in the existing ecosystem of crates/integration_tests, shielder-rust-sdk and shielder-relayer, under the existing PR checks.
There was a problem hiding this comment.
renamed to Shielder but kept in ShielderV0_1_0.sol file
There was a problem hiding this comment.
My point here is to change existing code while making sure that existing tests pass. Currently you're adding code outside the existing testing framework, is there any good reason for it?
There was a problem hiding this comment.
rewriting tests for new circuits is out of scope of this PR. let's not make it giant.
| /* | ||
| * Make an ERC-20 token deposit into the Shielder | ||
| */ | ||
| function depositERC20( |
There was a problem hiding this comment.
New contract code should be accompanied by test coverage. Our current way to do that is to use crates/integration-tests.
The skipping of integration tests was discussed in the context of Shielder SDK, not contract code coverage.
There was a problem hiding this comment.
note sure it's currently meaningful to write integration tests without withdraw. let's leave it for another PR
There was a problem hiding this comment.
The new deposit is compatible with the old withdraw and new account, as long as you only use token index 0. The tests I have in mind do not use withdraw.
There was a problem hiding this comment.
rewriting tests for new circuits is out of scope of this PR. let's not make it giant.
There was a problem hiding this comment.
the implementation looks very good, bu the versioning is attacking from all angles 😄 As much as I can understand naming the contracts with the semversion included, imo it's not needed all over the place, - it shoud be enough to do semversioning in the tags and in the Cargo.toml, which has the version field for this exact purpose
Implement Deposit function for token shortlist feature:
ShielderV_0_1_0TokenListtokenIndexinDepositEventzkOS-monorepo/contracts/ShielderV0_1_0.sol
Line 73 in 33a4a93
depositERC20zkOS-monorepo/contracts/ShielderV0_1_0.sol
Lines 232 to 241 in 33a4a93
zkOS-monorepo/contracts/ShielderV0_1_0.sol
Lines 387 to 396 in 33a4a93