Sdk v0.46.* and ibc-go v5.1.0 #349

hieuvubk · 2022-11-17T11:11:41Z

Closes: #347

Context and purpose of the change

This PR updates cosmos-sdk to v0.46.*. It shouldn't be considered ready for production until the release of v0.46.7, which deprecates all prior sdk releases.

Brief Changelog

update cosmos-sdk
update ibc-go

Author's Checklist

I have...

Run and PASSED locally all GAIA integration tests
If the change is contentful, I either:
- Added a new unit test OR
- Added test cases to existing unit tests
OR this change is a trivial rework / code cleanup without any test coverage

If skipped any of the tests above, explain.

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

reviewed state machine logic
reviewed API design and naming
manually tested (if applicable)
confirmed the author wrote unit tests for new logic
reviewed documentation exists and is accurate

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md?
This pull request updates existing proto field values (and require a backend and frontend migration)?
Does this pull request change existing proto field names (and require a frontend migration)?
How is the feature or change documented?
- not applicable
- jira ticket XXX
- specification (x/<module>/spec/)
- README.md
- not documented

into upgrade_ibc_v5

asalzmann · 2022-11-17T20:32:21Z

Is upgrade code also required here?

hieuvubk · 2022-11-18T16:40:48Z

Is upgrade code also required here?

Yes, we'll add upgrade handler after all bug is fixed

into upgrade_ibc_v5

x/icacallbacks/module_ibc.go

sampocs · 2023-01-06T19:28:02Z

x/icacallbacks/module_ibc.go

@@ -92,7 +94,11 @@ func (im IBCModule) NegotiateAppVersion(
 }

 // GetTxMsgData returns the msgs from an ICA transaction and can be reused across authentication modules
-func GetTxMsgData(ctx sdk.Context, ack channeltypes.Acknowledgement, logger log.Logger) (*sdk.TxMsgData, error) {
+func GetTxMsgData(ctx sdk.Context, ack channeltypes.Acknowledgement, logger log.Logger) (*types.ICATxResponse, error) {


If you're on board with moving the ack unmarshaling out, I think we should add it to the top of this function and rename it to something more generic like UnpackICAAcknowledgement(I'm sure you can think of a much better name)

And then each callback would take types.ICATxResponse as the argument instead of the Ack packet type

x/stakeibc/keeper/icacallbacks_undelegate.go

sampocs · 2023-01-06T19:31:42Z

x/icacallbacks/types/ICATxResponseStatus.go

+)
+type ICATxResponse struct {
+	Status ICATxResponseStatus // enum of SUCCESS, TIMEOUT, FAILURE
+	Data sdk.TxMsgData // TxMsgData


And then I think you want data to be a byte array here

x/icacallbacks/types/ica_tx_response.go

x/stakeibc/keeper/icacallbacks_undelegate.go

asalzmann · 2023-01-07T15:57:05Z

x/icacallbacks/module_ibc.go

 	// if ack is nil, a timeout occurred
-	if &ack == nil {
+	if &acknowledgement == nil {


Since we removed logging in x/records/module_ibc.go, should we add a log here so that it's easy to see how acks are being parsed?

yup, I was thinking that as well. Might make sense to add them in after testing though cause it's still a little unclear to me which branch will wind up being timeout, error, success, etc.
I'll add a TODO for the time being

x/icacallbacks/module_ibc.go

asalzmann · 2023-01-07T16:01:11Z

x/icacallbacks/module_ibc.go

+	//
+	// It's also unclear to me whether this timeout case below is possible
+	// In any case, we should just test this thoughly with the following cases:
+	//   Success


I don't think this sequence is possible, because the host chain should process either a timeout or an error, but not both

Yeah my biggest question is what a timeout looks like. It seems odd that we were identifying it by a nil ack. Fortunately, should be easy enough to test

We identify it with a nil ack out of convenience here https://github.com/notional-labs/stride/blob/acd8c57188652ce42c53a09c82a2d5bbe49b83af/x/stakeibc/module_ibc.go#L174

The timeout actually flows down a different code path (OnTimeout), so it probably makes sense to stop using a nil ack to identify timeouts and instead pass in an ICATxResponse with a timeout status. Sorry, should have caught this in the review - I'll push this up now.

Ah that's right, good call out!

asalzmann · 2023-01-07T16:01:51Z

x/icacallbacks/module_ibc.go

+	// so Acknowledgement_Result should only be returned if the transaction was successful
+	// However, I recall seeing a case where the acknowledgement type was Acknowledgement_Result but the transaction failed
+	//
+	// -> If my memory's correct, I think a failure on the host could come back as either an AcknowledgementError


also recall this - I think the way we tested it (as you mentioned below) was delegating to a non-existent validator?

sampocs · 2023-01-07T16:44:00Z

For testing, we can add logging at each step in the Unpack function.

And then we can test the following cases:

Transfer success
Transfer failure (hard code wrong delegation address in transfer deposit records step)
ICA success
ICA failure (hard code wrong validator address - must be a valid address but not be in the validator set)
ICA timeout (hard code TTL that's 1 second after current block time)

@asalzmann What am I forgetting? I think there might be two failure cases on the host - one that's rejected early by validate basic and one that's rejected after reading state from the store.
The ICA case above seems to handle the state failure. For a validate basic failure, maybe we send a Msg type that doesn't actually exist on the host or send a message with missing args?

And then for transfer I think the example above would be a validate basic failure - not sure how to invoke a state failure.

asalzmann · 2023-01-07T22:37:16Z

The above test plan makes sense - thoughts on doing this in a separate PR? This one is getting a bit hairy

I think we should probably write integration tests for these, wdyt?

sampocs · 2023-01-08T22:00:14Z

The above test plan makes sense - thoughts on doing this in a separate PR? This one is getting a bit hairy

Agreed, I don't think we should push any changes related to those tests, I think we can just do it locally. But I think we should avoid merging this to main until the integration tests pass. Maybe we get the acks working for the current v3 hosts, merge, then test a v5 host (evmos) and make relevant changes in another PR?

I think we should probably write integration tests for these, wdyt?

I think as far as the individual success/failure test cases, we should just determine the Ack packet format and then make unit tests. And then wrt integration tests, I think adding a v5 host is adequate.

…toring a channel

sampocs · 2023-01-11T00:06:51Z

@asalzmann Integration tests are working. I'm trying to test restoring the channel but looks like there's some bugs in the relayer we might have to solve first.
If the new changes look good, I think we should merge this.

Also I don't see any lint errors anymore, but lmk if there was something you saw that I might be missing.

asalzmann · 2023-01-11T16:52:09Z

x/icacallbacks/icacallbacks.go

+// ICA transactions have associated messages responses. IBC transfer do not.
+// With ICA transactions, the schema of the response differs depending on the version of ibc-go used,
+//     however, this function unifies the format into a common response (a slice of byte arrays)
+func UnpackAcknowledgementResponse(ctx sdk.Context, logger log.Logger, ack []byte, isICA bool) (*types.AcknowledgementResponse, error) {


Not needed here, in the future we might want to encode the ack type as an enum (ICA, Transfer, etc.) so that we can support more IBC apps

That's a great idea, much cleaner! I'll actually make a quick PR for that soon

asalzmann · 2023-01-11T17:07:43Z

LGTM

Co-authored-by: Son Trinh <sontrinh@Sons-MacBook-Pro.local> Co-authored-by: Jacob Gadikian <jacobgadikian@gmail.com> Co-authored-by: Aidan Salzmann <aidan@stridelabs.co> Co-authored-by: DongLieu <duongduydongtq@gmail.com> Co-authored-by: Nguyen Thanh Nhan <thanhnhan98qh@gmail.com> Co-authored-by: sampocs <sam.pochyly@gmail.com> Co-authored-by: sampocs <sam@stridelabs.co>

bump version: sdk => 0.46.4, ibc => 5.1.0

34d23df

hieuvubk marked this pull request as draft November 17, 2022 11:11

hieuvubk and others added 15 commits November 17, 2022 18:16

update version grpc

87a5a23

fix keychain import

33a6066

change import govtypes & store keys

c11dd19

fixing

2d82b3f

fix OnChanOpenInit

3c30499

NewIBCModule => NewIBCMiddleware

91a46f4

remove register rest api

66a4bd8

fix testutil network prunning

7c3ebd2

fix pruning testutil

5f81dd4

fix RegisterTendermintService

d92b389

Merge branch 'upgrade_ibc_v5' of https://github.com/notional-labs/stride

553c297

into upgrade_ibc_v5

fix app.go

a378c78

fix genaccount & main cmd

dad9eef

fix root.go in cmd

a5edb43

pull upstream

82d08c1

hieuvubk added 2 commits November 18, 2022 15:55

fix empty valset

b70b627

fixing

67fbece

hieuvubk and others added 9 commits November 21, 2022 15:08

get appVersion

9f7b932

fixing parse coin

e65581d

fix test for ibc denom

da8a4be

remove fmt

e43200c

fix merge confilct

52e9759

fix lint error

b7433c1

upgrade to v0.46.6 to avoid err

948675c

Merge branch 'upgrade_ibc_v5' of https://github.com/notional-labs/stride

ca2fe15

into upgrade_ibc_v5

add no lint check for sdk.MsgData

ec3fb68

sampocs reviewed Jan 6, 2023

View reviewed changes

sampocs added 2 commits January 6, 2023 15:54

refactored ica callback ack handling

2d185ab

updated relayer for v5

8e356f2

asalzmann reviewed Jan 7, 2023

View reviewed changes

pr comments and integration test fixes

36c379d

rm nil ack

af6f288

sampocs and others added 6 commits January 9, 2023 12:47

added debugging logs in ack unwrapper

28ffb8d

added unit tests for UnpackAck

d1af58c

added parse tx msg data function and more unit tests

025e373

fixed unit tests

782d385

Merge branch 'main' into upgrade_ibc_v5

167caee

added no lint annotation for sdk.MsgData

e9af2cf

mergify bot mentioned this pull request Jan 10, 2023

Upgrade ibc v5 #564

Merged

sampocs added 3 commits January 10, 2023 12:01

Merge remote-tracking branch 'upstream/main' into upgrade_ibc_v5

cfc9861

added logging helper to print ICA status

b6ce2d0

Merge remote-tracking branch 'upstream/main' into upgrade_ibc_v5

593429f

mergify bot mentioned this pull request Jan 10, 2023

v5 additional changes #565

Closed

fixed bug in counterparty connection with registering a host zone/res…

1717780

…toring a channel

sampocs requested a review from asalzmann January 10, 2023 23:32

fixed unit tests

5fc4413

asalzmann reviewed Jan 11, 2023

View reviewed changes

asalzmann approved these changes Jan 11, 2023

View reviewed changes

asalzmann merged commit e55f6f2 into Stride-Labs:main Jan 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sdk v0.46.* and ibc-go v5.1.0 #349

Sdk v0.46.* and ibc-go v5.1.0 #349

hieuvubk commented Nov 17, 2022 •

edited by faddat

Loading

asalzmann commented Nov 17, 2022

hieuvubk commented Nov 18, 2022

sampocs Jan 6, 2023

sampocs Jan 6, 2023

asalzmann Jan 7, 2023

sampocs Jan 7, 2023

asalzmann Jan 7, 2023

sampocs Jan 7, 2023

asalzmann Jan 7, 2023

sampocs Jan 8, 2023

asalzmann Jan 7, 2023

sampocs commented Jan 7, 2023 •

edited

Loading

asalzmann commented Jan 7, 2023 •

edited

Loading

sampocs commented Jan 8, 2023

sampocs commented Jan 11, 2023 •

edited

Loading

asalzmann Jan 11, 2023

sampocs Jan 11, 2023

asalzmann commented Jan 11, 2023

Sdk v0.46.* and ibc-go v5.1.0 #349

Sdk v0.46.* and ibc-go v5.1.0 #349

Conversation

hieuvubk commented Nov 17, 2022 • edited by faddat Loading

Context and purpose of the change

Brief Changelog

Author's Checklist

Reviewers Checklist

Documentation and Release Note

asalzmann commented Nov 17, 2022

hieuvubk commented Nov 18, 2022

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

Choose a reason for hiding this comment

sampocs commented Jan 7, 2023 • edited Loading

asalzmann commented Jan 7, 2023 • edited Loading

sampocs commented Jan 8, 2023

sampocs commented Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asalzmann commented Jan 11, 2023

hieuvubk commented Nov 17, 2022 •

edited by faddat

Loading

sampocs commented Jan 7, 2023 •

edited

Loading

asalzmann commented Jan 7, 2023 •

edited

Loading

sampocs commented Jan 11, 2023 •

edited

Loading