-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Generic 02-client cli cmds, removes light client specific ones #8259
Conversation
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'm getting the error
cannot unpack Any into ClientState *types.Any: failed unpacking protobuf message from Any
I think because the cache is not populating after unmarshaling an Any with JSON. I've come across this before. Is there any way to get the cache to populate?
@@ -480,10 +480,16 @@ func (s *IntegrationTestSuite) testQueryIBCTx(txRes sdk.TxResponse, cmd *cobra.C | |||
func (s *IntegrationTestSuite) TestLegacyRestErrMessages() { | |||
val := s.network.Validators[0] | |||
|
|||
// Write client state json to temp file, used for an IBC message. | |||
clientStateJSON := testutil.WriteToNewTempFile( |
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.
changing this since the cli command is being moved to a more generic one
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.
sounds good 👍
|
||
clientState := types.NewClientState(sequence, consensusState, allowUpdateAfterProposal) | ||
msg, err := clienttypes.NewMsgCreateClient(clientState, consensusState, clientCtx.GetFromAddress()) | ||
msg, err := types.NewMsgCreateClient(clientState, consensusState, clientCtx.GetFromAddress()) |
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.
Note: I intentionally do not return the client identifier generated since the call below is GenerateOrBroadcastTx
. This code cannot return a client identifier if the transaction is simply generated and not broadcasted. If users want this functionality, they should use external resources like a relayer. This should really only be for testing
Codecov Report
@@ Coverage Diff @@
## master #8259 +/- ##
==========================================
- Coverage 61.91% 61.91% -0.01%
==========================================
Files 609 609
Lines 35100 35101 +1
==========================================
Hits 21732 21732
- Misses 11084 11085 +1
Partials 2284 2284
|
NewCreateClientCmd(), | ||
NewUpdateClientCmd(), | ||
NewSubmitMisbehaviourCmd(), |
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 about update proposal and upgrade client?
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 add update client after the change to update client handling. The proposed approach would only require submitting a client identifier which is a lot less code than taking in a header
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.
proposals are included on the gov CLI afaik
if err != nil { | ||
return err | ||
} | ||
|
||
clientCtx = clientCtx.WithHeight(height) |
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.
Why is this no longer needed?
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.
PrintProto
doesn't print the context height. This code was likely introduced after confusion of looking at the previously existing rest code. The helper function for rest returns rest responses with the height injected into the response based on the context height. Since queries of a context height of 0 use the latest height, this was a necessary step to returning the correct height
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.
LGTM
Description
closes: #7874
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/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes