feat: introduce node command #2669
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tbruyelle
requested review from
ilgooz,
aljo242,
lumtis,
jeronimoalbi and
fadeev
as code owners
tbruyelle
force-pushed
the
issue-2489-node-bank-cmds
branch
2 times, most recently
from
08190bd
to
0ab22ac
Compare
|
Should we split down this PR to parts for ex:
|
The cosmosclient changes are nested with the previous changes of the original PR, so that's complicated... I can try! |
|
we don't need to worry much about this I think if it requires a ton of work, I'm in favor of not splitting |
tbruyelle
force-pushed
the
issue-2489-node-bank-cmds
branch
from
009538f
to
3792a26
Compare
Bounty address: cosmos1zra3wz596n0qc898myceka3q23x9rk8fuw65c7
Fixed a bunch of conflicts with recent renaming
That helps when some commands are not running properly.
Also simplify the logic by removing the read of the --from flag, which is no longer used in this command. BroadcastTx now takes a cosmosaccount.Account instead of just an account name. Since ensure the account comes from the keyring, and avoid the BroadcastTx impl to re-check that (because the keyring is always checked before the call to BroadcastTx).
fees are required, at least in cosmos-hub to send funds to an other account. W/o fees the transaction returns this message: error code: '13' msg: 'insufficient fees; got: required: 8uatom: insufficient fee' Also increase the additional gas amount added to the simulated gas, because I got some insufficient gas error with the previous value.
|
Can we resolve the existing conflicts? Reviewing now |
aljo242
reviewed
Aug 12, 2022
|
All commands fail with the default flag as post failed: Post "https://rpc.cosmos.network": dial tcp: address rpc.cosmos.network: missing port in addressCommands work if i use |
I can't reproduce the problem on my side, but I have noticed it sometimes ago with an other node, so let's add it 10bd29e |
aljo242
reviewed
Aug 13, 2022
aljo242
previously approved these changes
Aug 13, 2022
|
LGTM - just a formatting suggestion |
|
Tried to update the branch and created a merge error - my bad Fixed 3fa6677 |
aljo242
previously approved these changes
Aug 16, 2022
aljo242
approved these changes
Aug 18, 2022
ilgooz
reviewed
Aug 22, 2022
| func TestNodeQueryTx(t *testing.T) { | ||
| var ( | ||
| appname = randstr.Runes(10) | ||
| // alice = "alice" |
There was a problem hiding this comment.
Should remove these lines if not used?
Jchicode
pushed a commit
to Jchicode/cli
that referenced
this pull request
Aug 9, 2023
* add node command with bank query and send Bounty address: cosmos1zra3wz596n0qc898myceka3q23x9rk8fuw65c7 * Code review cleanup * Code review fix * fix merge * refactor * Merge branch 'develop' into issue-2489-node-banks-cmds Fixed a bunch of conflicts with recent renaming * wip: fix integration tests due to recent refac * test: finish apply refac in tests * Fix integration test on node command * various improvments in node integration test * test: read test.v to add more trace on step execution That helps when some commands are not running properly. * fix: spinner hides os keyring prompt * Ensure bank send can take account name or address Also simplify the logic by removing the read of the --from flag, which is no longer used in this command. BroadcastTx now takes a cosmosaccount.Account instead of just an account name. Since ensure the account comes from the keyring, and avoid the BroadcastTx impl to re-check that (because the keyring is always checked before the call to BroadcastTx). * fix after rebase * fix: bank balance can take an address absent of the keyring * fix tx broadcast error message * add fees flag to node tx bank send cmd fees are required, at least in cosmos-hub to send funds to an other account. W/o fees the transaction returns this message: error code: '13' msg: 'insufficient fees; got: required: 8uatom: insufficient fee' Also increase the additional gas amount added to the simulated gas, because I got some insufficient gas error with the previous value. * Change tx BroadcastMode to BroadcastSync Previous mode BroadcastBlock alweus triggered a timeout error, even if the tx was finally accepted in a block. RPC error -32603 - Internal error: timed out waiting for tx to be included in a block * fix linter * fix other lint * improve lookupAddress error report * fix typo * wip * simplify * add assertBankBalanceOutput * wip find a way to wait for block height * test: replace time.Sleep with app.WaitNBlocks * gofumpt * fix integration test * BroadcastBlock is deprecated * simplify node query * feat: move WaitForBlock methods in cosmosclient * fix: revert usage of BroadcastSync mode * docs: adapt blog tutorial to cosmosclient changes * feat: add --broadcasd-mode flag to node tx command The default value is still block but the flag can change to sync in order to avoid timeout when the tx is broadcasted to busy nodes. Also add a `node query tx` so the user can check when his tx is included in a block. * comments * style: consolidate arg names * fix: add port to default node * merge const decls * fix merge error * fix after merge * fix after merge Co-authored-by: Gjermund Bjaanes <gjermund@empower.eco> Co-authored-by: İlker G. Öztürk <ilkergoktugozturk@gmail.com> Co-authored-by: Alex Johnson <alex@shmeeload.xyz>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #2489
This PR continues the work of #2539. An other PR was required because the original branch was from an other repo.
What I did so far :
nodecommand integration testsnodecommand that hides the os keyring passphrase prompt. The problem exists because a spinner is displayed as soon ascliui.New()is invoked. Maybe other commands are impacted. c66131fbank send toAccount fromAccountimpl so it doesn't need the--fromflag and accepts both account name or address.--feesflag (required for tx on cosmos-hub apparently) cb4ab7c--broadcast-modeflag to let the user change it tosync, to avoid timeouts when the requested node is too busy e3b92cfnode q bank balancesandnode tx bank sendwith cosmos-hub account (see latest txs of https://www.mintscan.io/cosmos/account/cosmos1k2m58vv732yjrs6pnhaf3ar8dtwdql5mlcv478)time.Sleepin favor of new methodapp.WaitNBlocks()Kudos to @bjaanes who did the major part of the work!