Skip to content
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

Merged
merged 14 commits into from
Jan 8, 2021

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Jan 5, 2021

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.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

Copy link
Contributor Author

@colin-axner colin-axner left a 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(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @amaurymartiny

changing this since the cli command is being moved to a more generic one

Copy link
Contributor

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())
Copy link
Contributor Author

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
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #8259 (fb6d474) into master (51e7e91) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
x/ibc/core/02-client/module.go 25.00% <0.00%> (-8.34%) ⬇️
...light-clients/06-solomachine/types/misbehaviour.go 96.15% <ø> (ø)

Comment on lines +44 to +46
NewCreateClientCmd(),
NewUpdateClientCmd(),
NewSubmitMisbehaviourCmd(),
Copy link
Member

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?

Copy link
Contributor Author

@colin-axner colin-axner Jan 8, 2021

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

Copy link
Collaborator

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)
Copy link
Member

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?

Copy link
Contributor Author

@colin-axner colin-axner Jan 8, 2021

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

@colin-axner colin-axner added the A:automerge Automatically merge PR once all prerequisites pass. label Jan 8, 2021
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mergify mergify bot merged commit 0df0626 into master Jan 8, 2021
@mergify mergify bot deleted the colin/7874-client-cli branch January 8, 2021 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create general ibc light client CLI cmds
4 participants