IBC: bug fixes for creating IBC channels #206
Conversation
cmd/babylond/cmd/testnet.go
Outdated
| coins := sdk.Coins{ | ||
| sdk.NewCoin("testtoken", sdk.NewInt(1000000000)), | ||
| sdk.NewCoin("bbn", sdk.NewInt(1000000000000)), |
There was a problem hiding this comment.
This does not need to be added as NativeCoinMetadatas[0].Base uses the proper token (which is ubbn).
There was a problem hiding this comment.
Is there any way to use bbn here? since this account is used by the relayer who needs bbn
There was a problem hiding this comment.
Why does the relayer need to use bbn?
There was a problem hiding this comment.
It needs to submit txs to Babylon. Looks like we can only use bbn for submitting txs, right?
There was a problem hiding this comment.
No, we should use the denomination that is available. ubbn is the only denomination we support (and testtoken for the validator dirs generated by babylond testnet). This is similar to other chains (e.g. osmosis doesn't have an osmo denomination but only uosmo)
There was a problem hiding this comment.
For example to submit unbonding requests I always use ubbn, bbn is just the human readable format.
There was a problem hiding this comment.
Right, let me try if ubbn works in IBC playground
There was a problem hiding this comment.
It works 👍 Let's use ubbn then
| if err != nil { | ||
| _ = os.RemoveAll(outputDir) | ||
| return err | ||
| } | ||
|
|
||
| // save mnemonic words for this key | ||
| info := map[string]string{"secret": secret} |
There was a problem hiding this comment.
Why do we need to store the mnemonic? For rebuilding the wallet on another machine?
There was a problem hiding this comment.
When running locally, one can use the keyring location (without the mnemonic), since it contains the private key as well. When running on the infrastructure, we have three options:
- Manually identify the mnemonic and rebuild the wallet locally
- Copy the entire keyring from the deployment and move it locally (or just the private keys)
- Create a wallet locally and transfer funds to it
For testing with our partners we are going to use option 3. So I suggest we do the same for our tests.
There was a problem hiding this comment.
We need mnemonic for the relayer to import this wallet. Unfortunately relayer only supports importing via the mnemonic.
There was a problem hiding this comment.
I see. We can merge this for now and then on the IBC playground we can add methodology for generating a key and transferring funds to it from a Babylon node, as we should replicate the behavior that will happen on the real environment. This can also serve as pre-work for the Docker deployment.
There was a problem hiding this comment.
Agree. The current methodology is not that proper. In practice (eg demo) the relayer needs to create its own account and we need to give the relayer some funds.
| } | ||
|
|
||
| // timeout | ||
| curHeight := clienttypes.GetSelfHeight(ctx) | ||
| curHeight.RevisionHeight += 100 // TODO: parameterise timeout | ||
| timeoutTime := uint64(ctx.BlockHeader().Time.Add(time.Hour * 24).UnixNano()) // TODO: parameterise |
There was a problem hiding this comment.
This should come from a config file (or we can check out what other chains are doing). I suggest creating a Github issue so it doesn't get lost.
There was a problem hiding this comment.
Sure, created #207
| curHeight, // if the packet is not relayed after thit height, then the packet will be timeout | ||
| uint64(0), // no need to set timeout timestamp if timeout height is set | ||
| zeroheight, // no need to set timeout height if timeout timestamp is set | ||
| timeoutTime, // if the packet is not relayed after this time, then the packet will be timeout |
There was a problem hiding this comment.
| timeoutTime, // if the packet is not relayed after this time, then the packet will be timeout | |
| timeoutTime, // if the packet is not relayed after this time, then the packet will be timed out |
| if counterpartyVersion != types.Version { | ||
| return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: %s, expected %s", counterpartyVersion, types.Version) | ||
| } | ||
| // // TODO (Babylon): check version consistency (this requires modifying CZ code) |
There was a problem hiding this comment.
How do chains communicate with other chains that don't use the same IBC version? I would expect that there's a standard method for negotiating versions.
There was a problem hiding this comment.
In fact, this version is not the version of the IBC protocol, but the version of the IBC-based application. For example, it can be ics20-1 that denotes the first version of the ICS-20 fungible token transfer standard.
Since Babylon aims at establishing IBC channels with any Cosmos chains, Babylon should not impose any restriction on the version of the counterparty's module that implements IBC interfaces.
How do chains communicate with other chains that don't use the same IBC version?
For IBC itself, any two IBC-Go versions are compatible with each other.
There was a problem hiding this comment.
Then do we really need a TODO here?
There was a problem hiding this comment.
Once we go beyond phase 2 where we inject our code to CZs, we can create IBC connections with our module in CZs (rather than their existing ones) and have versioning in case of backward-incompatible updates.
| @@ -150,11 +151,6 @@ func (im IBCModule) OnAcknowledgementPacket( | |||
|
|
|||
| // this line is used by starport scaffolding # oracle/packet/module/ack | |||
|
|
|||
| var modulePacketData types.ZoneconciergePacketData | |||
There was a problem hiding this comment.
Why is the following check getting removed?
There was a problem hiding this comment.
Mostly 2 reasons: 1) we don't use the packet anywhere, and 2) formats of such packets are decided by CZs, and asserting them to types looks redundant due to 1).
This PR fixes multiple bugs that occur when I try to establish an IBC channel between Babylon and Cosmos Hub. Specifically,