Support SendToIbc event in smart contract #65

thomas-nguy · 2021-09-09T09:27:56Z

support ibc log event handler

PR Checklist:

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

yihuang · 2021-09-09T09:40:24Z

x/cronos/keeper/evm_log_handlers.go

@@ -30,11 +32,16 @@ var (
 	// EthereumTransferEvent represent the signature of
 	// `event __CronosSendToEthereum(address recipient, uint256 amount, uint256 bridge_fee)`
 	EthereumTransferEvent abi.Event
+
+	// IbcTransferEvent represent the signature of
+	// `event __CronosSendToIbc(address sender, string recipient, uint256 amount)`


Suggested change

// `event __CronosSendToIbc(address sender, string recipient, uint256 amount)`

// `event __CronosSendToIbc(string recipient, uint256 amount)`

It don't have the sender parameter in the definition.

yihuang · 2021-09-09T09:41:55Z

x/cronos/keeper/evm_log_handlers.go

@@ -136,7 +157,7 @@ func (h EthereumTransferHandler) Handle(ctx sdk.Context, contract common.Address
 	}

 	denom, found := h.cronosKeeper.GetDenomByContract(ctx, contract)
-	if !found {
+	if !found && !types.IsValidGravityDenom(denom) {


Suggested change

if !found && !types.IsValidGravityDenom(denom) {

if !found || !types.IsValidGravityDenom(denom) {

I guess it should be a || here?

yihuang · 2021-09-09T09:42:26Z

x/cronos/keeper/evm_log_handlers.go

+	}
+
+	denom, found := h.cronosKeeper.GetDenomByContract(ctx, contract)
+	if !found && !types.IsValidIBCDenom(denom) {


Suggested change

if !found && !types.IsValidIBCDenom(denom) {

if !found || !types.IsValidIBCDenom(denom) {

x/cronos/keeper/evm_log_handlers.go

calvinaco · 2021-09-09T13:08:24Z

x/cronos/keeper/evm_log_handlers_test.go

+			func() {
+			},


Suggested change

func() {

},

func() {},

x/cronos/keeper/evm_log_handlers.go

calvinaco · 2021-09-09T14:12:20Z

x/cronos/keeper/evm_log_handlers.go

+	if !found || !types.IsValidIBCDenom(denom) {
+		return fmt.Errorf("contract %s is not connected to native token", contract)
+	}


Should we distinguish the two cases?

!found => No mapping found

!types.IsValidIBCDenom(denom) => Mapping found, but the contract is not mapped to an IBC asset

calvinaco · 2021-09-09T14:13:16Z

x/cronos/keeper/evm_log_handlers.go

@@ -136,7 +157,7 @@ func (h EthereumTransferHandler) Handle(ctx sdk.Context, contract common.Address
 	}

 	denom, found := h.cronosKeeper.GetDenomByContract(ctx, contract)
-	if !found {
+	if !found || !types.IsValidGravityDenom(denom) {


Should we distinguish the two cases?

!found => No mapping found

!types.IsValidGravityDenom(denom)=> Mapping found, but the contract is not mapped to an gravity asset

calvinaco · 2021-09-09T15:05:45Z

x/cronos/keeper/evm_log_handlers.go

+
+	// IbcTransferEvent represent the signature of
+	// `event __CronosSendToIbc(string recipient, uint256 amount)`
+	IbcTransferEvent abi.Event


Shall we rename these variables based on the event name? i.e. SendToIbcEvent

I sometimes find it confusing when I look at IbcTransferEvent in the code

calvinaco · 2021-09-09T15:07:32Z

x/cronos/keeper/evmhooks_test.go

@@ -159,6 +163,47 @@ func (suite *KeeperTestSuite) TestEvmHooks() {
 				suite.Require().Equal(1, len(rsp.SendToEthereums))
 			},
 		},
+		{
+			"failed ibc transfer, invalid denom",


Suggested change

"failed ibc transfer, invalid denom",

"failed ibc transfer, non-IBC denom",

invalid is not self-explanatory here

calvinaco · 2021-09-09T15:21:10Z

x/cronos/keeper/evm_log_handlers_test.go

+		error     error
+	}{
+		{
+			"invalid denom, expect fail",


Suggested change

"invalid denom, expect fail",

"non-IBC denom, expect fail",

calvinaco · 2021-09-09T15:21:31Z

x/cronos/keeper/evm_log_handlers_test.go

+		error     error
+	}{
+		{
+			"invalid denom, expect fail",


Suggested change

"invalid denom, expect fail",

"non-Gravity denom, expect fail",

yihuang · 2021-09-10T06:55:19Z

contracts/src/CroBridge.sol

@@ -0,0 +1,14 @@
+pragma solidity ^0.6.11;


Should we remove this if it's not used?
if it's for integration testing, we could also move it to integration_tests/contracts/.

yihuang

LGTM overall, only minor suggestions.

codecov · 2021-09-10T15:36:03Z

Codecov Report

Merging #65 (fc9e5a2) into main (3ea70c5) will increase coverage by 3.61%.
The diff coverage is 38.27%.

@@            Coverage Diff             @@
##             main      #65      +/-   ##
==========================================
+ Coverage   21.51%   25.12%   +3.61%     
==========================================
  Files          27       32       +5     
  Lines        1729     2145     +416     
==========================================
+ Hits          372      539     +167     
- Misses       1324     1570     +246     
- Partials       33       36       +3

Impacted Files	Coverage Δ
app/app.go	`3.89% <0.00%> (-0.06%)`	⬇️
x/cronos/keeper/grpc_query.go	`0.00% <0.00%> (ø)`
x/cronos/keeper/hooks.go	`0.00% <0.00%> (ø)`
x/cronos/keeper/msg_server.go	`6.45% <0.00%> (ø)`
x/cronos/types/query.pb.gw.go	`0.00% <0.00%> (ø)`
x/cronos/types/tracer.go	`0.00% <0.00%> (ø)`
x/cronos/keeper/evm.go	`57.62% <66.66%> (+5.90%)`	⬆️
x/cronos/keeper/evmhooks.go	`80.00% <80.00%> (ø)`
x/cronos/keeper/evm_log_handlers.go	`86.52% <86.52%> (ø)`
x/cronos/keeper/ibc.go	`83.06% <100.00%> (+4.88%)`	⬆️
... and 9 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 66d9e40...fc9e5a2. Read the comment docs.

… denom

thomas-nguy requested a review from a team as a code owner September 9, 2021 09:27

thomas-nguy requested review from yihuang and calvinaco and removed request for a team September 9, 2021 09:27

thomas-nguy had a problem deploying to protected September 9, 2021 09:28 Failure

thomas-nguy changed the title ~~support transferToIbc log event in smart contract~~ support sendToIbc log event in smart contract Sep 9, 2021

yihuang reviewed Sep 9, 2021

View reviewed changes

thomas-nguy had a problem deploying to protected September 9, 2021 09:41 Failure

yihuang reviewed Sep 9, 2021

View reviewed changes

x/cronos/keeper/evm_log_handlers.go Show resolved Hide resolved

thomas-nguy had a problem deploying to protected September 9, 2021 09:53 Failure

thomas-nguy linked an issue Sep 9, 2021 that may be closed by this pull request

Support SendToIBC in evm_log_handlers #63

Closed

thomas-nguy had a problem deploying to protected September 9, 2021 10:03 Failure

thomas-nguy changed the title ~~support sendToIbc log event in smart contract~~ support SendToIbc event in smart contract Sep 9, 2021

thomas-nguy force-pushed the thomas/support-ibc-log-handler branch from 49196ea to 3312cc6 Compare September 9, 2021 12:01

thomas-nguy had a problem deploying to protected September 9, 2021 12:01 Failure

thomas-nguy changed the title ~~support SendToIbc event in smart contract~~ Support SendToIbc event in smart contract Sep 9, 2021

thomas-nguy changed the base branch from thomas/support-ibc-to-crc20 to main September 9, 2021 12:02

calvinaco reviewed Sep 9, 2021

View reviewed changes

yihuang reviewed Sep 10, 2021

View reviewed changes

yihuang approved these changes Sep 10, 2021

View reviewed changes

thomas-nguy had a problem deploying to protected September 10, 2021 15:33 Failure

yihuang had a problem deploying to protected September 13, 2021 01:09 Failure

thomas-nguy force-pushed the thomas/support-ibc-log-handler branch from c14207e to 0602ee4 Compare September 13, 2021 01:21

thomas-nguy force-pushed the thomas/support-ibc-log-handler branch from 0602ee4 to 265dd83 Compare September 13, 2021 01:22

thomas-nguy added 9 commits September 13, 2021 10:24

add unit test

d28a648

fix IbcTransferCoins logic for hook case

67eb2e2

support transferToIbc log event in smart contract

98ecb24

add solidity contracts

aa44971

fix comments and logic check

a07d7a4

update changelog

c156067

update CroBridge.sol

0ad0841

rename evm events

de9b933

fix tests

62858c7

thomas-nguy force-pushed the thomas/support-ibc-log-handler branch from 265dd83 to 62858c7 Compare September 13, 2021 01:32

thomas-nguy had a problem deploying to protected September 13, 2021 01:32 Failure

add different error message for invalid denom and non associated coin…

54b91af

… denom

thomas-nguy had a problem deploying to protected September 13, 2021 02:38 Failure

update CroBridge.sol solidity version

fc9e5a2

thomas-nguy had a problem deploying to protected September 13, 2021 02:53 Failure

thomas-nguy merged commit 0cfaaf9 into main Sep 13, 2021

yihuang deleted the thomas/support-ibc-log-handler branch September 13, 2021 03:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SendToIbc event in smart contract #65

Support SendToIbc event in smart contract #65

thomas-nguy commented Sep 9, 2021

yihuang Sep 9, 2021

thomas-nguy Sep 9, 2021

yihuang Sep 9, 2021

thomas-nguy Sep 9, 2021

yihuang Sep 9, 2021

thomas-nguy Sep 9, 2021

calvinaco Sep 9, 2021

calvinaco Sep 9, 2021

calvinaco Sep 9, 2021

calvinaco Sep 9, 2021

calvinaco Sep 9, 2021

calvinaco Sep 9, 2021

calvinaco Sep 9, 2021

yihuang Sep 10, 2021

yihuang left a comment

codecov bot commented Sep 10, 2021 •

edited

Loading

	// `event __CronosSendToIbc(address sender, string recipient, uint256 amount)`
	// `event __CronosSendToIbc(string recipient, uint256 amount)`

	if !found && !types.IsValidGravityDenom(denom) {
	if !found \|\| !types.IsValidGravityDenom(denom) {

	if !found && !types.IsValidIBCDenom(denom) {
	if !found \|\| !types.IsValidIBCDenom(denom) {

	"failed ibc transfer, invalid denom",
	"failed ibc transfer, non-IBC denom",

	"invalid denom, expect fail",
	"non-Gravity denom, expect fail",

Support SendToIbc event in smart contract #65

Support SendToIbc event in smart contract #65

Conversation

thomas-nguy commented Sep 9, 2021

PR Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 10, 2021 • edited Loading

Codecov Report

codecov bot commented Sep 10, 2021 •

edited

Loading