imp: fixed multi-chain support and added tests #215
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #215 +/- ##
=======================================
Coverage 98.68% 98.69%
=======================================
Files 11 11
Lines 531 535 +4
=======================================
+ Hits 524 528 +4
Misses 7 7 ☔ View full report in Codecov by Sentry. |
srdtrk
changed the title
| ChannelId: ibctesting.FirstChannelID, | ||
| CounterpartyChannelId: s.TendermintLightClientID, | ||
| Signer: simdUser.FormattedAddress(), | ||
| CounterpartyChannelId: ibctesting.FirstClientID, |
There was a problem hiding this comment.
I'm OK with hardcoding these, but let's not use these constants. They only make sense in a tendermint-only world. If we are going to use constants we should define some that have clearer names, because this is very hard to read and understand if it is actually correct.
There was a problem hiding this comment.
I think it is fine for now. We can rethink it in a refactor PR
| })) | ||
|
|
||
| s.Require().True(s.Run("Deploy ethereum contracts with SimdA client", func() { | ||
| os.Setenv(testvalues.EnvKeyTendermintRPC, simdA.GetHostRPCAddress()) |
There was a problem hiding this comment.
If this is easy to pull out into a helper function somewhere, it might be useful since we have the same code multiple places now.
There was a problem hiding this comment.
I'd say it is not easy. Maybe in a future refactor PR, we can think about how to organize a lot of this better
| transferCoin := sdk.NewCoin(simdA.Config().Denom, sdkmath.NewIntFromBigInt(transferAmount)) | ||
| denomOnEthereum := transfertypes.NewDenom(transferCoin.Denom, transfertypes.NewHop(packet.Payloads[0].DestPort, packet.DestChannel)) | ||
|
|
||
| ibcERC20Addr, err := s.ics20Contract.IbcERC20Contract(nil, denomOnEthereum.IBCDenom()) |
There was a problem hiding this comment.
Can you rename this so we don't have ibcERC20Addr and ibcERC20Address
There was a problem hiding this comment.
I'll rename it but same thing shows up in other tests as well, it's a short lived variable.
| @@ -233,14 +233,18 @@ contract ICS20Transfer is | |||
|
|
|||
| address erc20Address; | |||
| if (originatorChainIsSource) { | |||
| // we are the source of this token, so the denom should be the contract address | |||
| erc20Address = ICS20Lib.mustHexStringToAddress(packetData.denom); | |||
| bool isERC20Address; | |||
There was a problem hiding this comment.
Could you add a test that catches this so we both document the behavior and make sure there are no regressions on this?
There was a problem hiding this comment.
I actually wanna revisit this code and potentially change it. I'll create an issue for this as I believe likely the same issue which effects ibc-go also might be effecting this repo. And this should begin a new design discussion which should be out of scope for this PR. Therefore the issue I will create is going to:
- Test the new lines in solidity
- See if Users can directly transfer hashed IBC denoms using
MsgSendPacketibc-go#7848 effects this repo (and create tests for it in solidity) - See if this code can be cleaned up/refactored
Description
closes: #59
closes: #216
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.