-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
x/bank: Refactor CLI & Tests #6525
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6525 +/- ##
==========================================
+ Coverage 55.60% 56.81% +1.21%
==========================================
Files 457 481 +24
Lines 27440 28935 +1495
==========================================
+ Hits 15257 16439 +1182
- Misses 11083 11328 +245
- Partials 1100 1168 +68 |
string amount = 1 [(gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int", (gogoproto.nullable) = false]; | ||
// amount is the supply of the coin | ||
cosmos.Coin amount = 1 | ||
[(gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Coin", (gogoproto.nullable) = false]; |
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.
This returns an sdk.Coin
, which matches the querier behavior and other queries similar to this.
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.
Have you verified that setting node, keyring, etc. from the config file still works with these changes? I imagine it still works from other cli methods but not bank query and tx because the standard New
/Init
which uses viper is no longer called there.
It does not @aaronc which is a huge PIA. There is so much work to be done here. The CLI is such a cluster f#ck right now. However, I think I can manage to keep the current flow while working on the new. |
@alessio, @aaronc et al. I ask you that review this under the following assumptions:
|
…os-sdk into bez/6423-x-bank-cli-refactor
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.
🙈
Don't worry -- we're in good hands. I can see the light at the end of the tunnel... |
oh boy 🙈. anyway upward and onward! we'll make it happen. |
Description
Indent
from client logic (should have never existed -- sorry should be separate PR)TxGenerator
,Marshaler
andAccountRetriever
totestutil.Config
NewFactoryCLI
(NewFactoryFrom
needs to be removed)GenerateOrBroadcastTxCLI
(GenerateOrBroadcastTx
needs to be removed)ref: #6423
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 explorer