Conversation
gjermundgaraba
requested review from
AdityaSripal,
chatton,
colin-axner,
crodriguezvega and
damiannolan
as code owners
gjermundgaraba
requested review from
DimitrisJim,
bznein and
srdtrk
as code owners
|
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)
} |
colin-axner
left a comment
colin-axner
left a comment
There was a problem hiding this comment.
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
| true, | ||
| }, | ||
| { | ||
| "success, only localhost", |
There was a problem hiding this comment.
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
|
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
| types.ErrWasmAttributesNotAllowed, | ||
| }, | ||
| { | ||
| "failure: invalid clientstate type", |
There was a problem hiding this comment.
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.
moved over the impl from client state
damiannolan
left a comment
damiannolan
left a comment
There was a problem hiding this comment.
Looks great! Nice diffs 🔴 🔴
Thank you @gjermundgaraba and @colin-axner! 🙏🏻
| true, | ||
| }, | ||
| { | ||
| "success, only localhost", |
| 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.
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.
I will try and report back!
There was a problem hiding this comment.
looks to be fine. Will update all args unused in the module to be discarded
DimitrisJim
left a comment
DimitrisJim
left a comment
There was a problem hiding this comment.
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.
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
|
colin-axner
deleted the
gjermund/5959-make-localhost-a-stateless-implementation
branch
5 participants
Description
This PR makes the localhost light client stateless.
A couple of notes for reviwers:
RunMigrationsanyway.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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.