IBC content update prior to IDA cohort 3 #1274
Conversation
There was a problem hiding this comment.
Nice! Thanks for keeping the content up to date 🙏
Changes look good to me. Left mostly nits.
In the future, I would highly recommend splitting changes into multiple pr's when possible. I know as a developer it doesn't make much sense, but as a reviewer less diffs makes reviewing changes exponentially easier. Even pulling out all the * -> -, makes reviewing easier as my mind can separate changes which don't need extra consideration from changes that do (it makes the process smoother/faster)
academy/3-ibc/1-what-is-ibc.md
Outdated
| - The proof format that all implementations must produce and verify is defined in the [ICS-23 implementation documentation](https://github.com/cosmos/ibc/blob/master/spec/core/ics-023-vector-commitments/README.md). | ||
| <!-- Is this still up-to-date after dragonberry? --> | ||
|
|
There was a problem hiding this comment.
Are these taken from ibc-go docs? The proof format comment can be removed (it isn't useful/correct information). I think the ics23 spec itself isn't the easy to obtain information from
Light clients may decide on their own proof format (solo machine for example uses signatures)
There was a problem hiding this comment.
I checked and this was indeed taken from ibc-go docs. Will create an issue in the ibc-go repo for it
academy/3-ibc/5-token-transfer.md
Outdated
|
|
||
| ### Acknowledging or timing out packets | ||
|
|
||
| The reader is invited to try to find and analyze the code corresponding to this (remember that the place to start is the packet callbacks, usually defined in a file like `module_ibc.go` or `ibc_module.go`). |
There was a problem hiding this comment.
Hmm, are people using module_ibc.go? ibc_module.go should be recommend for consistency
There was a problem hiding this comment.
Ignite CLI uses module_ibc.go
There was a problem hiding this comment.
Great work, @tmsdkeys.
I couldn't comment on the corresponding lines, but I noticed that you could change UpdateClient for MsgUpdateClient.
I am also not sure the word initiation is the best one to use in sentences like The initiation of this handshake.... Would success maybe be better?
academy/3-ibc/4-clients.md
Outdated
| @@ -175,7 +173,13 @@ The next validator set is used for verifying subsequent submitted headers or upd | |||
|
|
|||
| The root is the **AppHash**, or the hash of the application state of the counterparty blockchain that this client is representing. This root hash is particularly important because it is the root hash used on a receiving chain when verifying [Merkle](https://en.wikipedia.org/wiki/Merkle_tree) proofs associated with a packet coming over IBC, to determine whether or not the relevant transaction has been actually been executed on the sending chain. If the Merkle proof associated with a packet commitment delivered by a relayer successfully hashes up to this `ConsensusState` root hash, it is certain that the transaction was actually executed on the sending chain and included in the state of the sending blockchain. | |||
|
|
|||
| The following is an example of how the Tendermint client handles this Merkle [proof verification](https://github.com/cosmos/ibc-go/blob/main/modules/core/23-commitment/types/merkle.go). The [ICS-23 spec](https://github.com/cosmos/ibc/tree/master/spec/core/ics-023-vector-commitments) addresses how to construct membership proofs, and the [ICS-23 implementation](https://github.com/confio/ics23) currently supports Tendermint IAVL and simple Merkle proofs out of the box. Note that non-Tendermint client types may choose to handle proof verification differently: | |||
| The following is an example of how the Tendermint client handles this Merkle [proof verification](https://github.com/cosmos/ibc-go/blob/main/modules/core/23-commitment/types/merkle.go). The [ICS-23 spec](https://github.com/cosmos/ibc/tree/master/spec/core/ics-023-vector-commitments) addresses how to construct membership proofs, and the [ICS-23 implementation](https://github.com/confio/ics23) currently supports Tendermint IAVL and simple Merkle proofs out of the box. | |||
There was a problem hiding this comment.
I think IAVL is independent from Tendermint (see here) and simple Merkle proofs is the one that is Tendermint specific, I think.
There was a problem hiding this comment.
Not sure about this, but when you check out the ICS-23 implementation lists:
- tendermint/iavl
- tendermint - SimpleMerkleProofs
There was a problem hiding this comment.
Ah, I see... But if you click on the link for tendermint/iavl then it goes to cosmos/iavl. Maybe the readme hasn't been updated for a long time and the IAVL repo was originally in the tendermint GitHub organization?
|
Thanks for the reviews @colin-axner @chatton @crodriguezvega ! Implemented most of your nits/suggestions.
Success would imply that the handshake runs through all steps right? Whereas the initiation is simply the trigger, the success or failure depending on the situation, no? @crodriguezvega |
Ah, I see what you mean then. Ok, then let's not change it. 👍 |
coldice
requested review from
coldice,
xavierlepretre,
gioelebonifetto and
8bitpal
as code owners
Documentation content update for Cosmos Concepts: IBC
This PR runs through the existing IBC content prior to the launch of IDA C3 and updates where necessary to accommodate for:
Further notes for reviewers: none as of now.
Change scope
The changes in this Pull Request include (please tick all that apply):