Skip to content

Tx Client Migration: x/staking#5915

Merged
alexanderbez merged 6 commits intocosmos:masterfrom
regen-network:sahith/staking-tx-proto
Apr 3, 2020
Merged

Tx Client Migration: x/staking#5915
alexanderbez merged 6 commits intocosmos:masterfrom
regen-network:sahith/staking-tx-proto

Conversation

@sahith-narahari
Copy link
Copy Markdown
Contributor

@sahith-narahari sahith-narahari commented Apr 2, 2020

References: #5864

Description


For contributor use:

  • 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

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 2, 2020

Codecov Report

Merging #5915 into master will decrease coverage by 0.57%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5915      +/-   ##
==========================================
- Coverage   57.06%   56.48%   -0.58%     
==========================================
  Files         342      342              
  Lines       20056    20261     +205     
==========================================
  Hits        11444    11444              
- Misses       7767     7972     +205     
  Partials      845      845
Impacted Files Coverage Δ
x/staking/client/cli/tx.go 6.44% <0%> (-5.4%) ⬇️
x/auth/client/tx.go 14.74% <0%> (ø) ⬆️

@sahith-narahari sahith-narahari marked this pull request as ready for review April 2, 2020 20:07
@sahith-narahari sahith-narahari marked this pull request as ready for review April 2, 2020 20:07
defaultMinSelfDelegation = "1"
)

// NewTxCmd returns a root CLI command handler for all x/slashing transaction commands.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// NewTxCmd returns a root CLI command handler for all x/slashing transaction commands.
// NewTxCmd returns a root CLI command handler for all x/staking transaction commands.

@aaronc
Copy link
Copy Markdown
Member

aaronc commented Apr 2, 2020

@alexanderbez did we decide we are or aren't migrating the tx REST handlers now? My sense based on our discussion around gRPC is that we were going to hold off on that now and think of a more holistic approach that includes that. Is that right?

@alexanderbez
Copy link
Copy Markdown
Contributor

alexanderbez commented Apr 3, 2020

Actually thinking about this more, I don't see how we can introduce gRPC and keep the legacy REST server around. That would only work if we keep all the legacy types and querier logic -- which we aren't. Correct me if I'm wrong here.

To answer your question, yes, this PR should have a migration for the TX REST handler, which you've done, thanks!

Copy link
Copy Markdown
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

LGTM -- great work @sahith-narahari 👍

@aaronc
Copy link
Copy Markdown
Member

aaronc commented Apr 3, 2020

Actually thinking about this more, I don't see how we can introduce gRPC and keep the legacy REST server around. That would only work if we keep all the legacy types and querier logic -- which we aren't. Correct me if I'm wrong here.

@alexanderbez Yes, we need to leave the old querier logic around, but I don't really see that as a problem - it can reuse the proto-generated types for parameters. We can see what it looks like in #5882 when I mark that R4R.

@alexanderbez
Copy link
Copy Markdown
Contributor

We will need to evaluate most likely via actual implementation and usage. It's hard to visualize it right now.

@alexanderbez alexanderbez merged commit 7f78e61 into cosmos:master Apr 3, 2020
WithAccountRetriever(ar)

txBldr, msg, err := BuildCreateValidatorMsg(cliCtx, txBldr)
cliCtx := context.NewCLIContextWithInputAndFrom(inBuf, args[0]).WithMarshaler(m)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be NewCLIContextWithInput

WithTxGenerator(txg).
WithAccountRetriever(ar)

cliCtx := context.NewCLIContextWithInputAndFrom(inBuf, args[0]).WithMarshaler(m)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be NewCLIContextWithInput

WithTxGenerator(txg).
WithAccountRetriever(ar)

cliCtx := context.NewCLIContextWithInputAndFrom(inBuf, args[0]).WithMarshaler(m)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be NewCLIContextWithInput

WithTxGenerator(txg).
WithAccountRetriever(ar)

cliCtx := context.NewCLIContextWithInputAndFrom(inBuf, args[0]).WithMarshaler(m)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be NewCLIContextWithInput

WithTxGenerator(txg).
WithAccountRetriever(ar)

cliCtx := context.NewCLIContextWithInputAndFrom(inBuf, args[0]).WithMarshaler(m)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be NewCLIContextWithInput

@clevinson clevinson added this to the v0.39 milestone May 5, 2020
@sahith-narahari sahith-narahari deleted the sahith/staking-tx-proto branch May 20, 2020 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants