Problem: missing high-level docs for x/cronos #183

yihuang · 2021-10-21T09:16:58Z

Closes: #64

Solution:

write cronos module spec

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

PR Checklist:

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

codecov · 2021-10-21T09:20:25Z

Codecov Report

Merging #183 (2b7dba4) into main (3ea70c5) will increase coverage by 5.21%.
The diff coverage is 55.18%.

@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
+ Coverage   21.51%   26.73%   +5.21%     
==========================================
  Files          27       33       +6     
  Lines        1729     2338     +609     
==========================================
+ Hits          372      625     +253     
- Misses       1324     1666     +342     
- Partials       33       47      +14

Impacted Files	Coverage Δ
app/prefix.go	`0.00% <0.00%> (ø)`
app/test_helpers.go	`0.00% <0.00%> (ø)`
x/cronos/keeper/grpc_query.go	`0.00% <0.00%> (ø)`
x/cronos/keeper/msg_server.go	`4.76% <0.00%> (-1.69%)`	⬇️
x/cronos/module.go	`59.64% <0.00%> (-2.17%)`	⬇️
x/cronos/types/codec.go	`0.00% <0.00%> (ø)`
x/cronos/types/events.go	`0.00% <ø> (ø)`
x/cronos/types/messages.go	`20.22% <ø> (+20.22%)`	⬆️
x/cronos/types/params.go	`57.35% <ø> (+3.78%)`	⬆️
x/cronos/types/proposal.go	`11.11% <ø> (ø)`
... and 21 more

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 9253a2d...2b7dba4. Read the comment docs.

thomas-nguy · 2021-10-21T09:23:38Z

should we include @lezzokafka and @aw126 to do proofreading

thomas-nguy · 2021-10-21T09:36:56Z

x/cronos/spec/01_concepts.md

+
+## Cronos
+
+Cronos is the name of Crypto.org EVM Chain, also the module that glue multiple components together.


Cronos is an EVM chain in Crypto.org ecosystem. It allows users to deploy and interact with smart contracts. Powered by Ethermint, it is built using cosmos SDK which allows to leverage to full capability of the cosmos ecosystem. It is also connected to the ethereum network with the gravity bridge integration (WIP).

Bridging asset from cosmos or ethereum are automatically converted to a CRC20 asset when moved to Cronos which make it extremely easy to integrate with existing web3 tools.

looks great, added.

x/cronos/spec/01_concepts.md

tomtau · 2021-10-21T09:46:46Z

x/cronos/spec/03_state_transitions.md

+
+# State Transitions
+
+<!-- define state transitions for accounts and storage -->


I guess this should define the token mapping management / hooks?

added sth about the CRUD of token mapping.

x/cronos/spec/01_concepts.md

JayT106 · 2021-10-21T18:57:44Z

x/cronos/spec/01_concepts.md

+
+## Cronos
+
+Cronos is the name of Crypto.org EVM Chain, also the name of the main module that glues multiple components together for Cronos chain.


maybe briefly describe which components have been used in Cronos

JayT106 · 2021-10-21T18:59:32Z

x/cronos/spec/01_concepts.md

+- When transferring CRO to Cronos chain, the decimal places of the amount are expanded to 18.
+- When transferring CRO from Cronos chain, the amount is truncated to 8 decimals, the remaining part is left in Cronos, so the total value is preserved.
+
+## Native Token


maybe introduce the Token system prior to Gas Token, move Gas Token under this section.

or separate the token system to an independent spec?

By token system, do you mean the difference between cosmos native tokens and evm tokens?

the native token you described after line 28. then move the gas token section (line 13) under Native token

JayT106 · 2021-10-21T19:01:40Z

x/cronos/spec/03_state_transitions.md

+- When a `TokenMappingChangeProposal` gov proposal is executed.
+- When the admin account execute a `MsgUpdateTokenMapping` message.
+
+There's no way to delete a token mapping.


maybe Highlight this?

is the token mapping part of the solidity contract or just in the store of the Cronos module?

It's in the storage of Cronos module.

maybe Highlight this?

added a Delete section for it.

JayT106 · 2021-10-21T19:11:36Z

x/cronos/spec/README.md

+order: 0
+title: EVM Overview
+parent:
+  title: "evm"


Do the titles need to be updated?

good catch, fixed.

x/cronos/spec/01_concepts.md

x/cronos/spec/02_state.md

x/cronos/spec/04_messages.md

x/cronos/spec/05_abci.md

tomtau

looking good 👍, I'd suggest running the text through some spell or grammar checking software (I skipped a few typos, as I guess it can be captured by https://github.com/tbroadley/spellchecker-cli grammarly etc.), but overall it's good.

two potentially good things to document are the expected upgrade behaviour + the expected Cosmos/Ethereum tooling usage, because I assume one needs to be careful there?

tomtau · 2021-10-22T03:17:45Z

x/cronos/spec/01_concepts.md

+
+Cronos is an EVM chain in Crypto.org ecosystem. It allows users to deploy and interact with smart contracts. Powered by Ethermint, it is built using cosmos SDK which allows to leverage to full capability of the cosmos ecosystem. It is also connected to the ethereum network with the gravity bridge integration (WIP).
+
+Bridging asset from cosmos or ethereum are automatically converted to a CRC20 asset when moved to Cronos which make it extremely easy to integrate with existing web3 tools.


maybe a link to CRC20 definition?

oh, it's briefly described below -- but may be good to link the actual crc20 doc describing the interface

Added the link to the contract itself for now, maybe document the crc20 later.

lets create a new repository and add the specification for CRC20 and transferable tokens through bridge

x/cronos/spec/01_concepts.md

tomtau · 2021-10-22T03:26:50Z

x/cronos/spec/01_concepts.md

+
+## Auto-deployed Contract
+
+A contract whose byte code is embedded in Cronos module and deployed by it, and some operations in it are only authorized to Cronos module. These contracts can be trusted by Cronos module directly. Currently, Cronos module support auto-deploy a minimal CRC20 contract to support automatic wrapping native token in EVM.


maybe linking the contract description or sketching out its code?

Added the link to the ModuleCRC20.sol.

tomtau · 2021-10-22T03:31:40Z

x/cronos/spec/07_params.md

+- `KeyIbcTimeout` The timeout value to use when Cronos module transfer tokens to IBC channels.
+
+- `KeyCronosAdmin` The account that is authorized to manage token mapping through message, empty means no admin, should be a valid bech32 cosmos address if specified.
+- `KeyEnableAutoDeployment` specifies if the auto-deployment feature is enabled.


what happens when auto-deployment is off and then it's turned on via paramchange proposal? maybe good to add some comments on these parameters whether they are easily "updatable" -- to me, it seems only CronosAdmin can be updated via paramchange... the others look like that they'd need some extra code to be executed for store migrations etc. (so perhaps there should be a warning that they are not easily updatable, but would need some custom upgrade logic?)

Added comment on the side effects of update at runtime, it seems only IbcCroDenom needs special care to migrate the tokens.
EnableAutoDeployment should be good, because when it's disabled at runtime, only the new contract deployment is disabled, token withdrawn is not.

tomtau · 2021-10-22T03:35:52Z

x/cronos/spec/04_messages.md

+
+## MsgUpdateTokenMapping
+
+Update external token mapping, insert if not exists, can only be called by Cronos admin account, which is configured in module parameters.


how is the supply managed if there was an existing token mapping, and it was updated to a different one?

one other thing: what if one maps several native tokens to one contract?

good catch, the issue is fixed here: #187
added a failure case in the doc.

tomtau · 2021-10-22T03:38:48Z

x/cronos/spec/04_messages.md

+
+## MsgTransferTokens
+
+Transfer IBC tokens away from Cronos chain, will do decimals conversion automatically for CRO.


maybe some details on what it does -- I assume it'll do the conversion and do some implicit action with ibc-transfer: https://github.com/cosmos/ibc-go/blob/main/proto/ibc/applications/transfer/v1/tx.proto#L20 with some of those fields hardcoded?

it actually calls the TransferKeeper.SendTranser directly:

cronos/x/cronos/keeper/ibc.go

Line 168 in 5e60034

return k.transferKeeper.SendTransfer(

Added doc about this, and the hardcoded timeout paramters.

tomtau · 2021-10-22T03:46:16Z

x/cronos/spec/04_messages.md

+- The coin denom is neither IBC nor gravity tokens.
+- The mapping does not exist and auto-deployment is not enabled.
+
+## MsgTransferTokens


is this the reverse of MsgConvertVouchers or how does one "exit" the EVM contract? what if the contract locks the tokens in some way?

Added comments that these messages are not supposed to be used by end-user normally, end-user can use the smart contract APIs, which might need to be documented at another time.

Currently, MsgTransferTokens only handles IBC tokens, while MsgConvertVouchers can also handle gravity tokens, so they are not the exact reverse.

how does one "exit" the EVM contract?

The message work on native token only, there's not much difference from using ibc-transfer message directly for non-cro tokens, for cro, it also does the decimal conversion.

tomtau · 2021-10-22T03:48:44Z

x/cronos/spec/04_messages.md

@@ -0,0 +1,36 @@
+<!-- order: 4 -->
+
+# Messages


one useful information for MsgConvertVouchers and MsgTransferTokens would be to specify from/to addresses which ecosystem they are expected to come from... I assume address in MsgConvertVouchers would the be used in EVM, so it should come from the Ethereum ecosystem, but when on first transfers in the IBC tokens, they may land on an address that comes from the Cosmos ecosystem?

or would those message check the account types?

done, documented the fields.

or would those message check the account types?

The from addresses is the message signer, so they must be normal accounts. other than that, there seems no other explicit check.

I assume address in MsgConvertVouchers would the be used in EVM

The underlying method of MsgConvertVouchers is called in ibc hooks, to convert the arrived ibc token to crc20.

tomtau · 2021-10-22T03:50:12Z

x/cronos/spec/03_state_transitions.md

+
+## Token Mapping
+
+### Create/Update


how are the supplies transitioned on updates?

Added a comment on that, I think the contract develop should be able to manage the token migration themselves, on the smart contract level.

Closes: crypto-org-chain#64 Solution: - write cronos module spec fix wordings add state transition update title highlight token mapping deletion Apply suggestions from code review Co-authored-by: aw126 <78806365+aw126@users.noreply.github.com> apply suggestions tmp Apply suggestions from code review Co-authored-by: Tomas Tauber <2410580+tomtau@users.noreply.github.com> remove Key prefix review suggstions Revert "remove Key prefix" This reverts commit 9fa8583a425f4cc7669fb2cb0c1ce29dcb484ea6. remove Key prefix review suggestions

thomas-nguy

looks good

yihuang requested a review from a team as a code owner October 21, 2021 09:16

yihuang requested review from JayT106 and thomas-nguy and removed request for a team October 21, 2021 09:16

yihuang requested a review from tomtau October 21, 2021 09:29

thomas-nguy reviewed Oct 21, 2021

View reviewed changes

tomtau requested a review from linfeng-crypto October 21, 2021 09:39

thomas-nguy reviewed Oct 21, 2021

View reviewed changes

x/cronos/spec/01_concepts.md Show resolved Hide resolved

tomtau reviewed Oct 21, 2021

View reviewed changes

thomas-nguy reviewed Oct 21, 2021

View reviewed changes

x/cronos/spec/01_concepts.md Show resolved Hide resolved

yihuang requested review from lezzokafka and aw126 October 21, 2021 09:58

JayT106 reviewed Oct 21, 2021

View reviewed changes