Support ibc to crc20

[thomas-nguy]

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

TODO

• Tests

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 #55 (ffd2369) into main (3ea70c5) will increase coverage by 5.17%.
The diff coverage is 83.22%.

@@            Coverage Diff             @@
##             main      #55      +/-   ##
==========================================
+ Coverage   21.51%   26.69%   +5.17%     
==========================================
  Files          27       29       +2     
  Lines        1729     1862     +133     
==========================================
+ Hits          372      497     +125     
- Misses       1324     1331       +7     
- Partials       33       34       +1     

Impacted Files	Coverage Δ
app/app.go	3.90% <0.00%> (-0.05%)	⬇️
x/cronos/keeper/msg_server.go	6.45% <0.00%> (ø)
x/cronos/keeper/evm.go	56.89% <33.33%> (+5.17%)	⬆️
x/cronos/keeper/evmhooks.go	80.00% <80.00%> (ø)
x/cronos/keeper/evm_log_handlers.go	88.29% <88.29%> (ø)
x/cronos/keeper/ibc.go	83.06% <100.00%> (+4.88%)	⬆️
x/cronos/keeper/keeper.go	100.00% <0.00%> (+5.66%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7980bd...ffd2369. Read the comment docs.

[tomtau]

I haven't kept up with previous PRs. This PR seems more manageable I can help to review, but I don't have the full context for review 😔 -- what's autoDeploy? do we have some design doc in the repo I can read to get more context?

[tomtau]

do contributions to this repo follow C4: https://rfc.zeromq.org/spec/42/#23-patch-requirements ? I can't get much context from the git commit log messages either😔 /~https://github.com/crypto-org-chain/cronos/commits/main

[yihuang]

I haven't kept up with previous PRs. This PR seems more manageable I can help to review, but I don't have the full context for review 😔 -- what's autoDeploy? do we have some design doc in the repo I can read to get more context?

The best doc for this probably is here: #24 (comment), we should put it into the repo somewhere I think.

[yihuang]

do contributions to this repo follow C4: https://rfc.zeromq.org/spec/42/#23-patch-requirements ? I can't get much context from the git commit log messages either😔 /~https://github.com/crypto-org-chain/cronos/commits/main

Currently, only some commits followed, let's enforce that from now on. 😄

[leejw51crypto]

if autoDeploy is false, how can it be activated?

[yihuang]

if autoDeploy is false, how can it be activated?

A gov module managed contract map.

[thomas-nguy]

Yeah sorry for that, I havent been following the C4 standard. Will add a better message on squash merge

[tomtau]

I haven't kept up with previous PRs. This PR seems more manageable I can help to review, but I don't have the full context for review 😔 -- what's autoDeploy? do we have some design doc in the repo I can read to get more context?

The best doc for this probably is here: #24 (comment), we should put it into the repo somewhere I think.

I've opened this issue: #64

I've read through that comment and still a bit unclear -- it says:

User call sendToCosmos on ethereum, gravity bridge sends the tokens to Cronos native tokens
Cronos side tries to lookup the token denom in the external_contracts map:

if exists, move the token to the contract.
if not exists, deploy a built-in contract, and set the connection in internal_contracts, move the token to the contract.

so, from that I'd assume "autoDeploy" should always be true -- so I'm not sure why that parameter is there? is it that for testing it's needed to be set to false?

[thomas-nguy]

so, from that I'd assume "autoDeploy" should always be true -- so I'm not sure why that parameter is there? is it that for testing it's needed to be set to false?

yes it is correct. In mainnet, we will not allow autodeployement. CRC20 tokens will be onboarded one by one following the steps:

1. deploy the CRC20 contract to cronos
2. enable the conversion by adding into the external_contracts map a new entry (gravityTokenDenom, CRC20address) through governance vote

The reason we don't want autodeployement is to have more control on the onboarding and also some contracts needs to implement additional logic than what the autodeployed contract provide.

The autodeploiement is mostly used for testing now.

[yihuang]

I haven't kept up with previous PRs. This PR seems more manageable I can help to review, but I don't have the full context for review 😔 -- what's autoDeploy? do we have some design doc in the repo I can read to get more context?

The best doc for this probably is here: #24 (comment), we should put it into the repo somewhere I think.

I've opened this issue: #64

I've read through that comment and still a bit unclear -- it says:

User call sendToCosmos on ethereum, gravity bridge sends the tokens to Cronos native tokens
Cronos side tries to lookup the token denom in the external_contracts map:

if exists, move the token to the contract.
if not exists, deploy a built-in contract, and set the connection in internal_contracts, move the token to the contract.

so, from that I'd assume "autoDeploy" should always be true -- so I'm not sure why that parameter is there? is it that for testing it's needed to be set to false?

There were discussions on slack that the business side might don't want this auto-deploy feature, thus will need a parameter for convenience. When disable auto-deploy feature, we only have the gov managed external contract mapping, when the user sends un-mapped tokens from ethereum, it'll end up in native tokens (not wrapped automatically).

[tomtau]

ok, thanks for the context! in general, it'd be good to have this basic rationale documented: #64 given Slack conversations are detached from this codebase and harder to search.

for autoDeploy, if it's indeed only for testing, I'd suggest pulling that code out into a private function instead of having it on a public API. If you foresee there may later be a need to flip it, then perhaps making it a network parameter makes sense -- just bear in mind x/params module is to be deprecated in the future SDK version (so that's something to possible list in ADR consequences).

overall, this looks ok -- public contracts aren't broken (2.6 in C4) as this wasn't released / isn't used externally yet, so one small nit would be to put a bit more helpful squashed commit message to document what problem this solved and how (instead of what changed)

[yihuang]

LGTM