-
Notifications
You must be signed in to change notification settings - Fork 586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: rewrite capability to not use sdk.Context #7089
Conversation
6adc989
to
228596b
Compare
memStore.Set(types.KeyMemInitialized, []byte{1}) | ||
memStore := k.memService.OpenMemoryStore(noGasCtx) | ||
if err := memStore.Set(types.KeyMemInitialized, []byte{1}); err != nil { | ||
panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
|
||
capability := types.NewCapability(index) | ||
for _, owner := range owners.Owners { | ||
// Set the forward mapping between the module and capability tuple and the | ||
// capability name in the memKVStore | ||
memStore.Set(types.FwdCapabilityKey(owner.Module, capability), []byte(owner.Name)) | ||
if err := memStore.Set(types.FwdCapabilityKey(owner.Module, capability), []byte(owner.Name)); err != nil { | ||
panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
|
||
// Set the reverse mapping between the module and capability name and the | ||
// index in the in-memory store. Since marshalling and unmarshalling into a store | ||
// will change memory address of capability, we simply store index as value here | ||
// and retrieve the in-memory pointer to the capability from our map | ||
memStore.Set(types.RevCapabilityKey(owner.Module, owner.Name), sdk.Uint64ToBigEndian(index)) | ||
if err := memStore.Set(types.RevCapabilityKey(owner.Module, owner.Name), sdk.Uint64ToBigEndian(index)); err != nil { | ||
panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tried to look into test failures for cap but could nail it down quite yet, will do so again later today
seems like e2e needs some additional tweaks judging from failures 👀
modules/apps/29-fee/keeper/escrow.go
Outdated
// cache context before trying to distribute fees | ||
cacheCtx, writeFn := ctx.CacheContext() | ||
sdkCtx := sdk.UnwrapSDKContext(ctx) // TODO: remove when Upgrading to 52 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering we wont be doing that environment workaround we talked about in call, can open a new issue for bumping to 0.52 and link in similar way. can take care of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#7223 lmk if this is okay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beautiful!
@@ -15,7 +15,7 @@ require ( | |||
cosmossdk.io/math v1.3.0 | |||
cosmossdk.io/x/upgrade v0.1.4 | |||
github.com/cometbft/cometbft v0.38.11 | |||
github.com/cosmos/cosmos-sdk v0.50.9 | |||
github.com/cosmos/cosmos-sdk v0.50.10-0.20240808075341-156231be8aef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this required for the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea need a function, we will tag again to get it, to help unblock this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what function was needed that wasn't included in v50.9?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think this lgtm, left comments for the todos which I'll commit now. much thanks again!
modules/apps/27-interchain-accounts/controller/keeper/account.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again!
Just a note, we will be removing capabilities for v10. #7107
I think its good to just get these changes into main right now tho. We can merge main back into the capability release branch and tag something when needed
Quality Gate passed for 'ibc-go'Issues Measures |
memStore := k.memService.OpenMemoryStore(ctx) | ||
isInitialized, err := memStore.Has(types.KeyMemInitialized) | ||
if err != nil { | ||
panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
Description
remove sdk.Context from capability and user kvstore service
when upgrading to 52 we will be able to drop comet as a dep here
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/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.