refactor: migrate from sdk.Context to Context.Context (2/n) #7081
Conversation
tac0turtle
changed the title
|
opening for preliminary review as i make sure tests are passing |
tac0turtle
requested review from
colin-axner,
AdityaSripal,
damiannolan,
srdtrk,
chatton,
DimitrisJim,
crodriguezvega,
gjermundgaraba and
bznein
as code owners
There was a problem hiding this comment.
Nice one, diffs look good. Left a few nits, myself and @DimitrisJim can address them.
Looks like there is a couple of failing tests in ICA which we'll have to look into before merging
| bz, err := store.Get(key) | ||
| if len(bz) == 0 { | ||
| return "", false | ||
| } | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
error handling and panic before len(bz) check?
There was a problem hiding this comment.
moved it as the logic was wrong above
|
|
||
| return string(store.Get(key)), true | ||
| return string(bz), true // todo: why the cast? |
There was a problem hiding this comment.
value is stored as []byte(channelID) where channelID is string
| bz, err := store.Get(key) | ||
| if len(bz) == 0 { | ||
| return "", false | ||
| } | ||
| if err != nil { | ||
| panic(err) | ||
| } |
|
|
||
| return string(store.Get(key)), true | ||
| return string(bz), true // todo: why the cast? |
There was a problem hiding this comment.
address value is stored as string... similar reasoning to the ics29 question you had on counterparty address, could store address bytes but string is more human readable
| has, err := store.Has(key) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| if !has { | ||
| return "", false | ||
| } | ||
|
|
||
| return string(store.Get(key)), true | ||
| bz, err := store.Get(key) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| return string(bz), true |
There was a problem hiding this comment.
I think we can optimise this for gas consumption similarly to one or two funcs on the other PR. cc @DimitrisJim
| bz, err := store.Get(key) | ||
| if len(bz) == 0 { | ||
| return "", false | ||
| } | ||
| if err != nil { | ||
| panic(err) | ||
| } |
| channelID := channeltypes.FormatChannelIdentifier(suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.GetNextChannelSequence(suite.chainA.GetContext())) | ||
| path.EndpointA.ChannelID = channelID | ||
|
|
||
| tc.malleate() // malleate mutates test data | ||
|
|
||
| chanCap, err = suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) | ||
| suite.Require().NoError(err) | ||
|
|
There was a problem hiding this comment.
I find it weird that I had to do this to fix failing tests here given the diffs, I can try to provide more info later, need more reasoning on it
There was a problem hiding this comment.
Let me try to explain this a little further.
The channel capability is generated in 04-channel (core IBC) and passed to application callback handlers (e.g. like this one under test - ica OnChanOpenInit).
This test calls keeper.OnChanOpenInit directly, rather than delivering a msg e2e, testing core integration points with the app - thus, the capability is generated directly in the test code here and passed to app callback.
From what I can see, this test as it existed, was creating a new capability with an empty channel ID. This is because the path.EndpointA.ChannelID field is not actually fulfilled here until after the INIT step of the handshake.
In the diffs, I get the next channel sequence nonce (monotonically increasing) and format a new channel ID with it. Then use that to create the new capability before invoking the app callbacks directly on the keeper.
It's moved under the call to tc.malleate() as some test cases create a channel and set it to closed, in order to test regaining control of an interchain account on a closed channel, i.e. without moving it we fix some tests, and cause failures in others surfaced from capability already being claimed for a particular channelID.
Hope that makes sense!
I'm still curious why this has only popped up now, and previously passed 👀
There was a problem hiding this comment.
The changes which caused the test failures to be raised in this PR are from GetActiveChannelID which previously allowed storing an empty string and have it being treated as a "found" or "truthy" value.
e.g. in testcase failure: different ordering from previous channel, there is a call to SetActiveChannelID in the malleate() func. It's actually setting an empty channel id as the active channel id on main, while store.Has() treats it as true.
|
There was a problem hiding this comment.
LGTM! Thank you @tac0turtle, and @DimitrisJim for fixing the other test!
There was a problem hiding this comment.
big thanks once again! Also thanks on quick failure debugging @damiannolan!
Description
second pr of migrating the repository to use context in public apis
ref: #5917
Example commit messages:
fix: skip emission of unpopulated memo field in ics20
deps: updating sdk to v0.46.4
chore: removed unused variables
e2e: adding e2e upgrade test for ibc-go/v6
docs: ics27 v6 documentation updates
feat: add semantic version utilities for e2e tests
feat(api)!: this is an api breaking feature
fix(statemachine)!: this is a statemachine breaking fix
-->
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.