Problem: evm contract can't call native module message by yihuang · Pull Request #45 · crypto-org-chain/cronos

yihuang · 2021-09-03T06:52:06Z

Closes #29

Solution:

implement bank and gravity evm hooks

👮🏻👮🏻👮🏻 !!!! 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-09-03T06:55:00Z

Codecov Report

Merging #45 (6c5bd94) into main (3ea70c5) will increase coverage by 4.13%.
The diff coverage is 83.19%.

@@            Coverage Diff             @@
##             main      #45      +/-   ##
==========================================
+ Coverage   21.51%   25.64%   +4.13%     
==========================================
  Files          27       29       +2     
  Lines        1729     1848     +119     
==========================================
+ Hits          372      474     +102     
- Misses       1324     1337      +13     
- Partials       33       37       +4

Impacted Files	Coverage Δ
app/app.go	`3.90% <0.00%> (-0.05%)`	⬇️
x/cronos/keeper/evmhooks.go	`80.00% <80.00%> (ø)`
x/cronos/keeper/evm_log_handlers.go	`88.29% <88.29%> (ø)`
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 0d6b1d6...6c5bd94. Read the comment docs.

thomas-nguy · 2021-09-06T07:20:53Z

+}
+
+func (h BankSendHook) PostTxProcessing(ctx sdk.Context, txHash common.Hash, logs []*ethtypes.Log) error {
+	for _, log := range logs {


could be cleanup a bit to loop only one time

I think instead of creating 2 hooks (for bank and gravity), you can define another interface which implement EventProcessing

Yeah, do you mean to register the log processors into sth like: event_id -> processer? sounds good.

Will the log/event order make a difference? And in some cases a processor may need to handle multiple events?

I just pushed a new implementation.

Will the log/event order make a difference? And in some cases a processor may need to handle multiple events?

Currently we only have one handler for one event, if we need to support more, can change the EventID method to return an array.

where is decimal conversion
10^8 -> 10^18 ?

#35
that is handled in this PR

Solution: - implement bank and gravity evm hooks update gomod2nix changelog test contract native call comment

leejw51crypto

lgtm

thomas-nguy · 2021-09-08T10:13:45Z

 	}
+
+	function test_native_transfer(uint amount) public {
+		emit __CronosNativeTransfer(msg.sender, amount);


Don't u have to burn the amount?

imo __CronosNativeTransfer is not clear for the operation. A transfer imply an origin and a destination.

Here it seems that you are either converting or minting?

It seems more that you are con

This is solely for testing the native call, the semantic of the call is to transfer mapped native token from contract address to recipient.
But because we can't setup token mapping in e2e testing right now, so the test itself is not complete yet (there's a TODO in that).

This is not the e2e testing for the complete bridge yet.

so maybe __CronosCosmosConvert or __CronosCosmosMint?

so maybe __CronosCosmosConvert or __CronosCosmosMint?

“Native” stands for native token, “Transfer” is also self explanatory I believe?

is also self explanatory I believe?

I dont think so..
"Native" has barely any meaning without context. And using "Transfer" without specifying a sender and destination sounds weird.

If we can't find a proper short term to describe the operation, I would prefer to be very explicit and call the event
"__CronosSendFromContractToAccount(receiver, amount)" instead...

Mind that most smart contract developer won't have deep knowledge about the internal implementation, or cosmos implementation

When I think about it again, can we say "From" is implied because the action is "Send" and we are executing a contract? It is the "Subject" (what token) and "To" that's unclear.

So

___CronosSendXToAccount // Account or Native Account? need to address in the terminology issue ___CronosSendXToEthereum // Send X (From the contract account) To Ethereum ___CronosSendXToIBCChannel // Similar to Gravity Bridge `SendToCosmos` but be more explicit about the token type

p.s. Having "From" is no harm, my motive is if we think event name is too long then we can think about to trim it or not. I am okay to include it and be more explicit. (Is there any restricting on event name length?)

When I think about it again, can we say "From" is implied because the action is "Send" and we are executing a contract? It is the "Subject" (what token) and "To" that's unclear.

So

___CronosSendXToAccount // Account or Native Account? need to address in the terminology issue ___CronosSendXToEthereum // Send X (From the contract account) To Ethereum ___CronosSendXToIBCChannel // Similar to Gravity Bridge `SendToCosmos` but be more explicit about the token type

p.s. Having "From" is no harm, my motive is if we think event name is too long then we can think about to trim it or not. I am okay to include it and be more explicit. (Is there any restricting on event name length?)

Looks good to me.
And let's use NativeTokens for X for now? or Coins?

Maybe 'X' can be omitted now?

__CronosSendToAccount
__CronosSendToEthereum
__CronosSendToIBCChannel

Even if we add NativeToken or Coins, it is not clear which coin is it. We need a proper term to designate the "Coin associated with the contract"

Maybe 'X' can be omitted now?

__CronosSendToAccount
__CronosSendToEthereum
__CronosSendToIBCChannel

Even if we add NativeToken or Coins, it is not clear which coin is it. We need a proper term to designate the "Coin associated with the contract"

Looks good too, I suppose we'll have detailed documentation about them as part of the CRC20 standard.

#62
opened issue and documented decision so far in it.

thomas-nguy · 2021-09-08T11:19:00Z

+
+	denom, found := h.cronosKeeper.GetDenomByContract(ctx, contract)
+	if !found {
+		return errors.New("contract is not connected to native token")


I am thinking if we could wrap add those errors in the ethereum log, it could be nice

can you add the contract address in the error?

I am thinking if we could wrap add those errors in the ethereum log, it could be nice

I believe that’s the case, the error returned by hook is saved in the return value of the EVM call.
But I need to double check on the rpc response.

https://github.com/tharsis/ethermint/blob/main/x/evm/keeper/state_transition.go#L198

Hmm, the detailed error won't appear in the response, the RPC user only sees a fixed error string.
But it do get logged in a logfile as an error.

okay I created a new issue for that #61

thomas-nguy

LGTM except for the event name convention.

yihuang requested review from JayT106, calvinaco, devashishdxt, leejw51crypto, thomas-nguy and tomtau as code owners September 3, 2021 06:52

thomas-nguy reviewed Sep 3, 2021

View reviewed changes

Comment thread x/cronos/keeper/evmhooks.go Outdated

thomas-nguy reviewed Sep 6, 2021

View reviewed changes

Problem: evm contract can't call native module message

9244e34

Solution: - implement bank and gravity evm hooks update gomod2nix changelog test contract native call comment

yihuang force-pushed the evmhook branch from 65bd6a0 to 9244e34 Compare September 8, 2021 06:52

yihuang requested a review from a team as a code owner September 8, 2021 06:52

yihuang requested a review from thomas-nguy September 8, 2021 06:54

HuangYi added 4 commits September 8, 2021 14:54

run gen-cronos-contracts

50b765e

rename fee_amount -> bridge_fee

b90b157

add evmhooks unit tests

eff32dd

run gen-cronos-contracts

85b0d5e

leejw51crypto approved these changes Sep 8, 2021

View reviewed changes

fix integration test

9416bab

thomas-nguy reviewed Sep 8, 2021

View reviewed changes

Comment thread x/cronos/keeper/evmhooks.go

HuangYi added 3 commits September 8, 2021 18:27

move EvmLogHandler to types module

0a4714d

move log handlers into separated file

bea3c97

ensure the interface

30d15bb

yihuang mentioned this pull request Sep 8, 2021

Problem: gravity bridge hooks and native token wrapping not connected #59

Merged

13 tasks

calvinaco reviewed Sep 8, 2021

View reviewed changes

Comment thread x/cronos/keeper/evm_log_handlers.go Outdated

thomas-nguy reviewed Sep 8, 2021

View reviewed changes

Comment thread x/cronos/keeper/evm_log_handlers.go Outdated

thomas-nguy reviewed Sep 8, 2021

View reviewed changes

Comment thread x/cronos/keeper/evm_log_handlers.go Outdated

thomas-nguy reviewed Sep 8, 2021

View reviewed changes

Comment thread x/cronos/keeper/evm_log_handlers.go

thomas-nguy reviewed Sep 8, 2021

View reviewed changes

Comment thread x/cronos/keeper/evm_log_handlers.go Outdated

fix comments, const, error messages

b492b32

yihuang mentioned this pull request Sep 8, 2021

Problem: there's no consensus on terminology used for tokens #60

Open

add ethereum(gravity bridge) transfer unit tests

593189c

thomas-nguy approved these changes Sep 9, 2021

View reviewed changes

rename event

6c5bd94

yihuang merged commit f7980bd into crypto-org-chain:main Sep 9, 2021

yihuang deleted the evmhook branch September 9, 2021 02:01

coderabbitai Bot mentioned this pull request Apr 19, 2024

Problem: panic on invalid elasticity_multiplier #1397

Merged

13 tasks

Uh oh!

Conversation

yihuang commented Sep 3, 2021

PR Checklist:

Uh oh!

codecov Bot commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leejw51crypto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihuang Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomas-nguy Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihuang Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thomas-nguy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Sep 3, 2021 •

edited

Loading

yihuang Sep 8, 2021 •

edited

Loading

thomas-nguy Sep 8, 2021 •

edited

Loading

yihuang Sep 9, 2021 •

edited

Loading