-
Notifications
You must be signed in to change notification settings - Fork 579
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
feat: make 09-localhost stateless #6683
feat: make 09-localhost stateless #6683
Conversation
Hmm, or would it somehow be better to keep a separate module account like the staking module does? // GetBondedPool returns the bonded tokens pool's module account
func (k Keeper) GetBondedPool(ctx context.Context) (bondedPool sdk.ModuleAccountI) {
return k.authKeeper.GetModuleAccount(ctx, types.BondedPoolName)
}
// GetNotBondedPool returns the not bonded tokens pool's module account
func (k Keeper) GetNotBondedPool(ctx context.Context) (notBondedPool sdk.ModuleAccountI) {
return k.authKeeper.GetModuleAccount(ctx, types.NotBondedPoolName)
} |
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.
Looks great! Left various comments as I think we can simplify things even just a little more. I can push the changes when I get a moment
req = &types.QueryClientStatesRequest{} | ||
}, | ||
true, | ||
}, | ||
{ | ||
"success, only localhost", |
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.
grpc should no longer return localhost in its query. There's no information to obtain, as it would just return back the height of the request
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.
that makes sense!
I am updating the code to remove the localhost client state entirely. The localhost module will now be a single file (really 2 functions) 🎉 |
…ke-localhost-a-stateless-implementation
@@ -486,21 +483,6 @@ func (suite *KeeperTestSuite) TestWasmSudo() { | |||
}, | |||
types.ErrWasmAttributesNotAllowed, | |||
}, | |||
{ | |||
"failure: invalid clientstate type", |
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.
this became a duplicate test case
clientState, found := getClientState(clientStore, cdc) | ||
if !found { | ||
return errorsmod.Wrap(clienttypes.ErrClientNotFound, clientID) | ||
// ensure the proof provided is the expected sentinel localhost client proof |
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.
moved over the impl from client state
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.
Looks great! Nice diffs 🔴 🔴
Thank you @gjermundgaraba and @colin-axner! 🙏🏻
req = &types.QueryClientStatesRequest{} | ||
}, | ||
true, | ||
}, | ||
{ | ||
"success, only localhost", |
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.
that makes sense!
clientStore := l.storeProvider.ClientStore(ctx, exported.LocalhostClientID) | ||
clientStore.Set(host.ClientStateKey(), clienttypes.MustMarshalClientState(l.cdc, &clientState)) | ||
return nil | ||
func (LightClientModule) Initialize(ctx sdk.Context, clientID string, clientState, consensusStateBz []byte) error { |
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.
unsure if its necessary but could _
unused args, we had a linter complain at this at some point before (I'm not sure we still use it tho)
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.
I will try and report back!
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.
looks to be fine. Will update all args unused in the module to be discarded
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.
looks amazing! Great work both!
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
…' of github.com:cosmos/ibc-go into gjermund/5959-make-localhost-a-stateless-implementation
…ke-localhost-a-stateless-implementation
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.
unsure if we want to delete this file, but left for now
…' of github.com:cosmos/ibc-go into gjermund/5959-make-localhost-a-stateless-implementation
Quality Gate passed for 'ibc-go'Issues Measures |
Description
This PR makes the localhost light client stateless.
A couple of notes for reviwers:
RunMigrations
anyway.closes: #5959
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.