Upgrade IBC to v4.2.0 #3838

nicolaslara · 2022-12-22T14:03:01Z

What is the purpose of the change

Update ibc to v4. This is required to make the wasm hooks reusable.

Brief Changelog

Upgrade IBC to v4.2.0
(as a consequence) Upgrade wasm dependency to v0.30.0

Testing and Verifying

All tests still pass

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes)
How is the feature or change documented? (not applicable )

ValarDragon · 2022-12-23T17:37:41Z

osmoutils/ibc.go

+// NOTE: Acknowledgements are written into state and thus, changes made to error strings included in packet acknowledgements
+// risk an app hash divergence when nodes in a network are running different patch versions of software.
+func NewStringErrorAcknowledgement(err string) channeltypes.Acknowledgement {
+	// ToDo: Do we want to do this or do we want to use the IBC errors and emit the string?


I don;t understand the situation enough to have any opinions here

is there any particular reason you guys have chosen to use this in favour of channeltypes.NewErrorAcknowledgement?

Acknowledgements are written into the ibc store on RecvPacket so that the counterparty can verify their existence on AcknowledgePacket, and delete commitments (and in the case of ordered channels, increment an expected sequence).

Suppose an error string which is included in a packet acknowledgment is modified in a patch version update of an osmosis release. Nodes running lets say v14.0.0 and those running v14.0.1 will produce different app hashes.

We made this change to NewErrorAcknowledgement in ibc-go/v4 as we deemed it unsafe as developers may unknowingly change an error string value which could lead to the above. Also, to note, error values returned from protobuf codecs UnmarshalJSON produce non-deterministic output. So for example, when unmarshaling packet data []byte to a well known type in OnRecvPacket its not safe to include the error in the ack. See for example we omit the error in transfer acks. I think you guys are using encoding/json in places here tho, which I hope is a bit more sane than the proto shenanigans.

☝️ @ValarDragon This is the reason. I think it makes sense to change it, but I didn't want to do this without a discussion because it changes the behaviour (errors in ack vs errors in events).

I think a middle way is a good option, where we can add some information in the acks, but only from this packet and not from underlying errors.

I don't se why unmarshalling the error would lead to non-determinism as it's just a string (though I'm sure I'm wrong)

IIRC its because the jsonpb unmarshaling uses maps. Ranging on these maps in the deserialisation process will be non deterministic and therefore, you may get a different key returned in the error msg for a particular struct value.

that makes sense. And I think that aligns with only using string errors if they do not rely on errors from other parts of the stack.

I like having data in acks (errors in this case, but the success could also have data) because it allows us to do communication in cases where events aren't available (cosmwasm). I think the Async-ICQ implementation does something similar. But you're right about the risk of halts if we're not careful when changing these.

I should refactor the code so that it's very hard to accidentally introduce changes to the merkelized errors in minor versions. This change will probably need to happen both here and in the rate limit middleware/contract

ValarDragon · 2022-12-23T17:38:56Z

x/ibc-hooks/ibc_module.go

@@ -44,12 +44,12 @@ func (im IBCMiddleware) OnChanOpenInit(
 		hook.OnChanOpenInitBeforeHook(ctx, order, connectionHops, portID, channelID, channelCap, counterparty, version)
 	}

-	err := im.App.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, channelCap, counterparty, version)
+	finalVersion, err := im.App.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, channelCap, counterparty, version)


Not sure what this final version is, but ACK that its wired right

OnChanOpenInit returns the version. I'm also not sure, but I think that is the modified version. I'm passing both to the After method in case it needs to act on either. We could also ignore it and if someone needs it they can use Override... just thought it was weird not to pass it down since we're passing err.

ValarDragon · 2022-12-23T17:39:40Z

x/ibc-rate-limit/testutil/chain.go

-	app.BeginBlock(abci.RequestBeginBlock{Header: header})
 	gInfo, res, err := app.Deliver(txCfg.TxEncoder(), tx)

-	app.EndBlock(abci.RequestEndBlock{})
-	app.Commit()


LGTM, though I don't see any relation to rest of the PR

This function overrides a function from the ibc testutils. They changed it so begin/end block must be called before and after, so if we keep this here all tests fail because of calling those twice

ValarDragon

LGTM

* initial changes to migrate to ibc v4 * added checksum to proposal * begin and end block are now being called inside nextBlock * added changelog * linked pr on changelog * remove local replace * using error acks from osmoutils * osmoutils tagged * go sum * added checksum

…14 (#3925) * Upgrade IBC to v4.2.0 (#3838) * initial changes to migrate to ibc v4 * added checksum to proposal * begin and end block are now being called inside nextBlock * added changelog * linked pr on changelog * remove local replace * using error acks from osmoutils * osmoutils tagged * go sum * added checksum * feat(x/twap): modify cli to add geometric option (#3812) * feat(x/twap): geometric twap code gen query boilerplate * revert cli change * query gen * wire up API * test * fix * fixes * cli * lint * refactor via flag * refactor * refactor * fixes * lint * add arithmetic twap alias * Make wasm hooks importable (#3850) * moved ibc-hooks to its own go.mod * updated ibc hooks version * go sum * add ics23 patch into x/ibc-hooks * Fix wasm import version conflict * Update x/ibc-hooks to osmoutils v0.0.2 * Update versions Co-authored-by: Dev Ojha <[email protected]> * refactor(x/twap): handle spot price error case in the context of geometric twap (#3845) * refactor(x/twap): handle spot price error case * supporting test cases * table-driven log tests * test(x/twap): add randomized geometric twap test on a balancer pool (#3844) * test(x/twap): add randomized test with a balancer pool * comments * multiplicative tolerance, fewer retries and larger initial supply range * Basic geometric twap e2e test (#3835) * feat(x/twap): geometric twap code gen query boilerplate * revert cli change * query gen * wire up API * test * fix * fixes * add geometric queries * create pool.json * add test * resolve conflict * fix: swap uosmo in * fix problem with wallet creation * updates * simplify and add comments * Update tests/e2e/e2e_test.go * Update tests/e2e/e2e_test.go * Update tests/e2e/configurer/chain/queries.go Co-authored-by: Adam Tucker <[email protected]> * Update tests/e2e/configurer/chain/queries.go Co-authored-by: Adam Tucker <[email protected]> * Update tests/e2e/e2e_test.go Co-authored-by: Adam Tucker <[email protected]> * Update tests/e2e/e2e_test.go Co-authored-by: Adam Tucker <[email protected]> * Update tests/e2e/e2e_test.go Co-authored-by: Adam Tucker <[email protected]> * Update tests/e2e/e2e_test.go Co-authored-by: Adam Tucker <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Adam Tucker <[email protected]> * feat(x/twap): whitelist GeometricTwap and GeometricTwapToNow (#3852) * feat(x/twap): GeometricTwap and GeometricTwapToNow queries added to Stargate whitelist * update docs * fix(scripts): proto gen for osmoutils (#3854) * fix(scripts): proto gen for osmoutils * Update scripts/protocgen.sh * fix(scripts): proto gen osmoutils path (#3859) * added packet timeouts to wasm hooks (#3862) * add negative to cli (#3888) * Making osmoutils compile on chains that don't use our SDK fork (#3899) * making osmoutils compile on chains that don't use osmosis' fork of the cosmos sdk * updated imports for e2e tests * go fumpt * updated version everywhere * added changelog entry * remove deprecation from arithmetic & geometric twap to now query (#3917) * Add types & boilerplate for the Downtime detector module (#3609) Sub-component of #3603 Adds types for the thin module intended for downtime detection - Add downtime detection module types No tests added - Does this pull request introduce a new feature or user-facing behavior changes? somewhat - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? yes - How is the feature or change documented? In its spec * Add downtime detector module (#3688) * WIP * Switch to enum * Remove params query * Add query * Add wiring, add import/export test * Start begin block test * Finish keeper tests * Add CLI * Wire downtime detector CLI + queries * more module wiring * add types test * Fix state alteration test * Fix superfluid test * Add stargate whitelist support * Apply code comment * Rename folder * Add requested godoc * Update x/downtime-detector/genesis.go Co-authored-by: Adam Tucker <[email protected]> * Apply adam suggestion for having a `-` * move genesis test to own file Co-authored-by: Adam Tucker <[email protected]> * Initial by hand fixes * feat(osmomath): Exp2 function (#3708) * feat(osmomath): exp2 function * export exp2 * changelog * refactor ErrTolerance to use Dec instead of Int for additive tolerance * Update osmomath/exp2.go * Update osmomath/exp2.go * Update osmomath/exp2.go * Update osmomath/exp2_test.go * Update osmomath/exp2_test.go * do bit shift instead of multiplication * godoc about error bounds * comment about bit shift equivalency * merge conflict * improve godoc * typo * remove TODOs - confirmed obsolete * Runge's phenomenon comment * [x/TWAP] Expose a geometric TWAP API (#3529) * refactored twap api.go for geometric TWAP * added barebon docs * romans feedback * new * fix * nichola feedback * final roman comments * fix twap by hand * change to gamm * fix balancer test * bump to v14 upgrade * e2e fix * add remaining diff from main to ibc-rate-limit * update contracts test * osmomath: `AddMut` and `QuoMut` (#3779) * mut add * test add mut * quo mut * test quo mut/ remove want from test struct * refactor exp * change mutatives code * change * not allocaing * exp change to quomut * remove file * refactor quo * refactor ad * refactor tests * Modify CHANGELOG * Whitelist EstimateSwapExactAmountOut (#3693) * whitelist EstimateSwapExactAmountOut * doc: changelog * updated rate limit contract * Fix rust checks (#3576) * added cargo.lock * added Cargo.lock as an artifact * added new bytecode with lock file Co-authored-by: Nicolas Lara <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Hieu Vu <[email protected]> Co-authored-by: Roman <[email protected]> Co-authored-by: Supanat <[email protected]> Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Sishir Giri <[email protected]> Co-authored-by: Ruslan Akhtariev <[email protected]> Co-authored-by: mattverse <[email protected]> Co-authored-by: ByeongSu Hong <[email protected]>

nicolaslara added 3 commits December 22, 2022 14:26

initial changes to migrate to ibc v4

39e1268

added checksum to proposal

8ae0d2c

begin and end block are now being called inside nextBlock

3865742

github-actions bot added the C:app-wiring Changes to the app folder label Dec 22, 2022

nicolaslara added 2 commits December 22, 2022 15:04

added changelog

cb6c8a2

linked pr on changelog

30bcd1b

nicolaslara marked this pull request as draft December 22, 2022 14:06

nicolaslara added 3 commits December 22, 2022 15:09

remove local replace

a62ab67

using error acks from osmoutils

6d60498

Merge branch 'main' into nicolas/ibc_4

9da75bf

nicolaslara changed the title ~~Nicolas/ibc 4~~ Upgrade IBC to v4.2.0 Dec 22, 2022

nicolaslara added the V:state/breaking State machine breaking PR label Dec 22, 2022

nicolaslara added 3 commits December 22, 2022 16:34

osmoutils tagged

7896efb

go sum

4b5601e

added checksum

b2623bf

nicolaslara marked this pull request as ready for review December 23, 2022 15:58

ValarDragon reviewed Dec 23, 2022

View reviewed changes

ValarDragon approved these changes Dec 23, 2022

View reviewed changes

ValarDragon merged commit a0505dc into main Dec 23, 2022

ValarDragon deleted the nicolas/ibc_4 branch December 23, 2022 17:40

nicolaslara mentioned this pull request Jan 3, 2023

Use standard IBC errors #3908

Merged

github-actions bot mentioned this pull request Mar 1, 2024

Broken Links Detected #7641

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade IBC to v4.2.0 #3838

Upgrade IBC to v4.2.0 #3838

nicolaslara commented Dec 22, 2022 •

edited

Loading

ValarDragon Dec 23, 2022

damiannolan Dec 23, 2022 •

edited

Loading

nicolaslara Dec 24, 2022

damiannolan Dec 24, 2022

nicolaslara Dec 24, 2022

ValarDragon Dec 23, 2022

nicolaslara Dec 24, 2022

ValarDragon Dec 23, 2022

nicolaslara Dec 24, 2022

ValarDragon left a comment

Upgrade IBC to v4.2.0 #3838

Upgrade IBC to v4.2.0 #3838

Conversation

nicolaslara commented Dec 22, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

Choose a reason for hiding this comment

damiannolan Dec 23, 2022 • edited Loading

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

ValarDragon left a comment

Choose a reason for hiding this comment

nicolaslara commented Dec 22, 2022 •

edited

Loading

damiannolan Dec 23, 2022 •

edited

Loading