Problem: bi-directional token conversion is not implemented

[thomas-nguy]

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

Solution: Implement ADR-008

Todo: add test

PR Checklist:

• Have you read the CONTRIBUTING.md?
• Does your PR follow the C4 patch requirements?
• Have you rebased your work on top of the latest master?
• Have you checked your code compiles? (make)
• Have you included tests for any non-trivial functionality?
• Have you checked your code passes the unit tests? (make test)
• Have you checked your code formatting is correct? (go fmt)
• Have you checked your basic code style is fine? (golangci-lint run)
• If you added any dependencies, have you checked they do not contain any known vulnerabilities? (go list -json -m all | nancy sleuth)
• If your changes affect the client infrastructure, have you run the integration test?
• If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
• If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
• If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.

Thank you for your code, it's appreciated! :)

[codecov]

Codecov Report

Merging #600 (c133e3d) into main (9fef7fa) will increase coverage by 1.22%.
The diff coverage is 53.77%.

@@            Coverage Diff             @@
##             main     #600      +/-   ##
==========================================
+ Coverage   39.12%   40.35%   +1.22%     
==========================================
  Files          36       36              
  Lines        1848     1990     +142     
==========================================
+ Hits          723      803      +80     
- Misses       1071     1117      +46     
- Partials       54       70      +16     

Impacted Files	Coverage Δ
x/cronos/keeper/gravity_hooks.go	0.00% <0.00%> (ø)
x/cronos/keeper/msg_server.go	4.54% <0.00%> (ø)
x/cronos/proposal_handler.go	5.88% <0.00%> (-0.37%)	⬇️
x/cronos/types/proposal.go	10.34% <0.00%> (-0.77%)	⬇️
x/cronos/types/types.go	38.09% <23.52%> (-61.91%)	⬇️
x/cronos/keeper/evm_log_handlers.go	78.36% <26.66%> (-4.03%)	⬇️
x/cronos/genesis.go	64.28% <50.00%> (+2.74%)	⬆️
x/cronos/keeper/evm.go	54.88% <54.83%> (-3.12%)	⬇️
x/cronos/keeper/ibc.go	75.91% <62.50%> (-7.29%)	⬇️
x/cronos/keeper/keeper.go	76.33% <85.00%> (+10.27%)	⬆️
... and 7 more

[yihuang]

integration test need to be updated, the code changes looks good.

[yihuang]

Code changes looks good, can you add an integration test case to boost confidence?

[thomas-nguy]

integration tests for ibc added there
https://github.com/crypto-org-chain/cronos/pull/609/files

[yihuang]

LGTM

[FinnZhangCrypto]

Based on the whole code, looks all good to me. Not sure if the DSToken code is the same as the current ModuleCRC20 token in cronos-token-init-launch repo, if so then all good.

Additional question about the isSource flag:

• in the current version, this flag can only be initialized once in constructor when it was deployed. It means we cannot switch contract model to the previous version of ModuleCRC20. Is that an expected feature? Can you provide more details?
• I'm not sure if the module_address will perform a burn operation in actual script(guess it should do to keep consistence with previous version?). And please ensure the actual burn operation() after triggered the corresponding __CronosSendToIbc or __CronosSendToChain event.
• Can you explain what this module_address actually is?

[FinnZhangCrypto]

Or I guess this module_address will transfer or burn the corresponding tokens according to the actual feature(to chain or to ibc). Like this below:

• to ibc: transfer tokens from user to module_address, then module_address burn tokens to keep consistence with previous version.
• to chain: transfer tokens from user to module_address, then module_address transfer to Gravity Bridge to the target chain(e.g. Ethereum).

Please correct me if I'm wrong. Thanks!

[FinnZhangCrypto]

Or I guess you will use module_address to do different operations according to the actual use?
Based on the previous ModuleCRC20 contract, all tokens will be burn when sending tokens:

• to ibc: 2 kinds of methods to do that, transfer to module_address then burn / transfer to module_address and directly to ibc(when getting back, directly transfer back to user, doesn't need to mint)
• to chain: 2 kinds of methods to do that, transfer to module_address then burn(mint tokens on GB side, when getting back, directly burn GB side and mint in this CRC21 side) / the same as above.

[thomas-nguy]

to clarify about the source boolean.

Source define is the contract token represent an asset originated from the chain (cronos) like VVS, FERRO etc or comes from another chain (ETH, WBTC, ATOM)

It should be defined during initialisation and should probably not change.

module_address is a special address use by the module when performing an evm operation. The module will invoke the smart contract method bypassing the orignal transaction flow and the "msg.sender" address will be set to module_address. It holds certains priviledge in the contract such as be able to mint or burn tokens freely or move tokens

The reason we are not "burning/minting" tokens for source contract but move them instead is that we want to preserve the total supply of the tokens (calculated by total_supply())
For non-source tokens, burning is okay because the total supply is defined on the original contract (not in cronos)

[yihuang]

Can you explain what this module_address actually is?

module_address is only used in:

require(msg.sender == module_address);

which guarantees it can only be called by chain native code, rather than anyone else.