cosmos / ibc-go

Inter-Blockchain Communication Protocol (IBC) implementation in Golang.
https://ibc.cosmos.network/
MIT License
544 stars 576 forks source link

add info regarding middleware pitfalls #5132

Open Reecepbcups opened 10 months ago

Reecepbcups commented 10 months ago

It does not seem this repository makes note of some pretty large design implications. This issue outlines 2 issues we at Strangelove have encountered in the past month along side the Amulet team. Other developers seem to have run into similar issues so it would be good to get a list outlined for how to build safe middlewares.

1) IBC v7.1.X+ made no mention of the newly registered invariant total-escrow-per-denom in the migration guide, changelog, or release notes. You have to dig into the adr to go find the changes. This doc does not state there is a registered invariance if you do not update the state entry for these tokens. You have to find that either by looking at the code, or a chain halt for this reason.

State breaking changes like this should be at least noted so we don't find it after a halt due to an addition.

This affected our v7.0.0 PFM middleware instance (Pigeonfall).

2) ModuleCdc.UnmarshalJSON should have a note somewhere it has non-determistic errors. While IBC-Go discards this in its returned ack error, it is not known until it's hit. These err's are useful for debugging but the "see events for details" does is not helpful when you have more than 1 middleware (wrapped multiple times). So you are forced to use the unsafe err to debug the issue.

This halted osmosis testnet (Viperstrike) & was posted about in the CosmosSDK slack channel. Also affected 2 other large cosmos chains within their own modules built.

Request:

Can we get a guide that outlines the pitfalls for building out a middleware on ibc? 1 and 2 are good starting points but I am sure there are more as well. These things will only get worse as more begin to build middlewares and more IBC apps. We can't assume developers will know these things until it's too late.

srdtrk commented 7 months ago

I'll grow this list more but let me add some more pitfalls for a future guide: