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

Tx CLI proto module interface #5989

Merged
merged 58 commits into from
May 21, 2020
Merged

Tx CLI proto module interface #5989

merged 58 commits into from
May 21, 2020

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Apr 13, 2020

Summary

ref: #5864

This adapts the GetTxCmd method on AppModuleBasic to support the new tx cmd's we created in #5864 , simplifies those cmd's, and makes them amino compatible (with passing integration tests).

Details

  • AppModuleBasic.GetTxCmd now takes a single CLIContext parameter
  • renamed ClientTx -> TxBuilder and moved it as well as associated interfaces (like AccountRetriever) to client/context
  • added NodeQuerier parameters to AccountRetriever methods
  • adds TxGenerator and AccountRetriever fields to CLIContext
  • renamed CLIContext.Marshaler -> CLIContext.JSONMarshaler
  • adds InitWithInputAndFrom, etc. methods to CLIContext
  • updated all module tx CLI & REST cmd's created in Transaction Client (CLI & REST) Protobuf Migration #5864, wired up in the modules and deleted deprecated code
  • updated ADR 020 to reflect the changes

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

codecov bot commented Apr 28, 2020

Codecov Report

Merging #5989 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5989   +/-   ##
=======================================
  Coverage   55.66%   55.66%           
=======================================
  Files         447      447           
  Lines       26866    26866           
=======================================
  Hits        14956    14956           
  Misses      10843    10843           
  Partials     1067     1067           

@lgtm-com
Copy link

lgtm-com bot commented Apr 28, 2020

This pull request introduces 1 alert when merging d0cd45a into 6469447 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

x/staking/client/rest/tx.go Outdated Show resolved Hide resolved
x/staking/client/rest/tx.go Outdated Show resolved Hide resolved
x/staking/client/rest/tx.go Outdated Show resolved Hide resolved
x/gov/client/cli/tx.go Outdated Show resolved Hide resolved
x/gov/client/cli/tx.go Outdated Show resolved Hide resolved
@aaronc
Copy link
Member Author

aaronc commented Apr 28, 2020

@alexanderbez I started working on #5864 (comment) here. If you're able to take a quick look let me know if this approach works.

I realize that almost every new CLI method is touched by the refactoring already. Wondering if I should just go ahead and refactor those methods like I did for x/bank in the same PR or leave that for another PR?

@alexanderbez
Copy link
Contributor

@aaronc there are a lot of moving pieces going on at this point. I'm having trouble keeping track. Are you referring to the use of any? Shouldn't we wrap up migrating modules first?

@aaronc
Copy link
Member Author

aaronc commented Apr 29, 2020

@aaronc there are a lot of moving pieces going on at this point. I'm having trouble keeping track. Are you referring to the use of any? Shouldn't we wrap up migrating modules first?

@alexanderbez this is related to module tx CLI not Any, but since I'm waiting for feedback on various pieces there I was looking for something else that looked low hanging. But we can wait on this until later if it's easier to keep track.

@alexanderbez
Copy link
Contributor

If we move to any, which it seems like we are (I presumed we had consensus). Wouldn't that render a lot of the CLI design discussion no longer that useful? It doesn't make sense to use all of these interfaces and generators when we can just go back to a StdTx, right? Or do you still plan on keeping the current design?

@aaronc
Copy link
Member Author

aaronc commented May 18, 2020

If we move to any, which it seems like we are (I presumed we had consensus). Wouldn't that render a lot of the CLI design discussion no longer that useful? It doesn't make sense to use all of these interfaces and generators when we can just go back to a StdTx, right? Or do you still plan on keeping the current design?

So far I'm thinking it makes sense to keep the current design because at a very minimum it lets us safely support proto and amino transactions from the same code base. Which, I think for now is a requirement.

One thing I am noticing, is that with this design (both the tx and module design), we should probably be passing using the pointer version of CLIContext everywhere.

The only existing instance of AccountRetriever needs a CLIContext that has the node connection details setup. But if we construct the CLIContext and AccountRetriever before the tx command, the node connection details won't be populated in AccountRetriever and it will just fail. Passing around *CLIContext and just letting it be mutated would solve that.

In general I appreciate using the functional approach and not mutating things, but doesn't seem like accomplishes much in this case. There are other cases I've seen where maybe *CLIContext would be simpler.

What do you think?

@aaronc
Copy link
Member Author

aaronc commented May 18, 2020

The alternative to *CLIContext everywhere would be making CLIContext or NodeQuerier a parameter of the AccountRetriever methods.

@alexanderbez
Copy link
Contributor

We cannot pass a reference to a CLIContext -- so I'd opt for arguments in this case as you've suggested.

@aaronc aaronc added the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label May 20, 2020
@aaronc aaronc marked this pull request as ready for review May 20, 2020 19:07
@aaronc aaronc added the R4R label May 20, 2020
@aaronc aaronc removed the WIP label May 20, 2020
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.

utACK

client/context/account_retriever.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
client/context/context.go Outdated Show resolved Hide resolved
client/context/tx_generator.go Show resolved Hide resolved
aaronc and others added 3 commits May 21, 2020 17:03
Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Copy link
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.

utACK

@alexanderbez alexanderbez added the A:automerge Automatically merge PR once all prerequisites pass. label May 21, 2020
@mergify mergify bot merged commit 850419f into master May 21, 2020
@mergify mergify bot deleted the aaronc/5864-module-interface branch May 21, 2020 21:29
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* WIP

* WIP

* WIP on removing x/auth dependency from client/tx

* Revert unneeded changes

* Simplify cli tx UX

* Wire up bank tx REST routes

* Fix assignment issue

* Wire up bank NewSendTxCmd

* fix lint

* revert file

* revert file

* fix simcli

* Refactor AccountRetriever

* Fix build

* Fix build

* Fix build

* Fix integration tests

* Fix tests

* Docs, linting

* Linting

* WIP on all modules

* Implement other module new tx cmd's

* Fix cmd's

* Refactor existing GetTxCmd

* Fix cmd

* Removing deprecated code

* Update ADR 020 & CHANGELOG

* Lint

* Lint

* Lint

* Lint

* Lint

* Lint

* Lint

* Fix client/tx tests

* Fix mocks

* Fix tests

* Lint fixes

* REST tx migration

* Wire up REST

* Linting

* Update CHANGELOG, docs

* Fix tests

* lint

* Address review feedback

* Update CHANGELOG.md

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

* Update CHANGELOG.md

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

* group vars

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
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 T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants