feat: add node command with bank query and send #2539

gjermundgaraba · 2022-05-30T08:55:15Z

Bounty address: cosmos1zra3wz596n0qc898myceka3q23x9rk8fuw65c7

gjermundgaraba · 2022-05-30T08:56:20Z

ignite/cmd/node_query.go

+func flagSetPagination(query string) *flag.FlagSet {
+	fs := flag.NewFlagSet("", flag.ContinueOnError)
+
+	fs.Uint64(flagPage, 1, fmt.Sprintf("pagination page of %s to query for. This sets offset to a multiple of limit", query))


Flags and the code below is pretty much a copy paste from cosmos-sdk

gjermundgaraba · 2022-05-30T08:57:45Z

ignite/cmd/node_tx.go

+	flagGenerateOnly = "generate-only"
+
+	gasFlagAuto   = "auto"
+	flagGasPrices = "gas-prices"


I opted to only include gas and gas-prices. The gas-adjustment flag doesn't seem to be necessary since we already have logic in cosmosclient to increase gas when using "auto"

gjermundgaraba · 2022-05-30T08:58:04Z

ignite/cmd/node_tx.go

+
+func flagSetGasFlags() *flag.FlagSet {
+	fs := flag.NewFlagSet("", flag.ContinueOnError)
+	fs.String(flagGasPrices, "", "Gas prices in decimal format to determine the transaction fee (e.g. 0.1uatom)")


Most of these strings are just copy pasted from cosmos-sdk

gjermundgaraba · 2022-05-30T08:58:59Z

ignite/pkg/cosmosaccount/cosmosaccount.go

@@ -51,7 +51,7 @@ const (

 // Registry for accounts.
 type Registry struct {
-	homePath           string


I renamed this to avoid confusion. This is really the keyring dir, and was already used as such.

gjermundgaraba · 2022-05-30T09:00:43Z

ignite/pkg/cosmosclient/consensus.go

@@ -0,0 +1,66 @@
+package cosmosclient


I moved this out of the cosmosclient file so that any non-generic functionality is separated into files

gjermundgaraba · 2022-05-30T09:02:44Z

ignite/pkg/cosmosclient/cosmosclient.go

-func (c Client) Status(ctx context.Context) (*ctypes.ResultStatus, error) {
-	return c.RPC.Status(ctx)
-}
+func performQuery[T any](c Client, q func() (T, error)) (T, error) {


This is just to have a wrapper function that locks the bech32 prefix for queries. See how it is used in the bank client

gjermundgaraba · 2022-05-30T09:03:39Z

ignite/pkg/cosmosclient/cosmosclient.go

 func (c Client) BroadcastTxWithProvision(accountName string, msgs ...sdktypes.Msg) (
-	gas uint64, broadcast func() (Response, error), err error) {
+	unsignedTx client.TxBuilder, broadcast func() (Response, error), err error) {


I changed for this to return the unsigned transaction instead so that this could be used for generate-only commands. You simply don't call the broadcast function.
You can still get the gas from the unsigned tx.

gjermundgaraba · 2022-05-30T09:04:28Z

integration/account/cmd_account_test.go

+
+const testAccountMnemonic = "develop mansion drum glow husband trophy labor jelly fault run pause inside jazz foil page injury foam oppose fruit chunk segment morning series nation"
+
+func TestAccount(t *testing.T) {


I just realized there were no integration tests for account, so I added some (they are really fast)

ilgooz

Works like a charm! 🎊 👏

ilgooz · 2022-05-31T11:00:57Z

ignite/cmd/node_query.go

+}
+
+func getPagination(cmd *cobra.Command) (*query.PageRequest, error) {
+	pageKey, _ := cmd.Flags().GetString(flagPageKey)


How about wrapping al these under a single var block, so it can be better fmt'ed?

ilgooz · 2022-05-31T11:05:52Z

ignite/pkg/cosmosclient/bank.go

+	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
+)
+
+func (c Client) BankBalances(address string, pagination *query.PageRequest) (sdk.Coins, error) {


Suggested change

func (c Client) BankBalances(address string, pagination *query.PageRequest) (sdk.Coins, error) {

func (c Client) BankBalances(ctx context.Context, address string, pagination *query.PageRequest) (sdk.Coins, error) {

and same for others

ilgooz · 2022-05-31T11:06:11Z

ignite/pkg/cosmosclient/bank.go

+	}
+
+	_, err := c.BroadcastTx(fromAccountName, msg)
+	if err != nil {


Suggested change

if err != nil {

return err

ilgooz · 2022-05-31T11:11:07Z

ignite/cmd/node_query_bank_balances.go

+func NewNodeQueryBankBalances() *cobra.Command {
+	c := &cobra.Command{
+		Use:   "balances [address]",
+		Short: "Query for account balances by address",


Suggested change

Short: "Query for account balances by address",

Short: "Query for account balances by account name or address",

ilgooz · 2022-05-31T11:11:27Z

ignite/cmd/node_query_bank_balances.go

+
+func NewNodeQueryBankBalances() *cobra.Command {
+	c := &cobra.Command{
+		Use:   "balances [address]",


Suggested change

Use: "balances [address]",

Use: "balances [account-name|address]",

ilgooz · 2022-06-02T10:33:14Z

ignite/pkg/cosmosclient/bank.go

+		Pagination: pagination,
+	}
+
+	resp, err := performQuery[*banktypes.QueryAllBalancesResponse](c, func() (*banktypes.QueryAllBalancesResponse, error) {


Cool generics!

ilgooz · 2022-06-02T10:43:58Z

ignite/pkg/cosmosclient/cosmosclient.go


-	commit, err := node.Commit(ctx, &height)
+func (c Client) GenerateTx(accountName string, msgs ...sdktypes.Msg) (string, error) {


Suggested change

func (c Client) GenerateTx(accountName string, msgs ...sdktypes.Msg) (string, error) {

What if we

drop this func

rename/refactor BroadcastTxWithProvision to CreateTx (because it doesn't really broadcast by default)

func CreateTx(accountName string, msgs ...sdktypes.Msg) (txService TxService, err error) type TxService struct { client.TxBuilder } // Gas is gas decided to use for this tx. // either calculated or configured by the caller. func (s TxService) Gas() uint64 {} // Broadcast signs and broadcasts this tx. func (s TxService) Broadcast() error {} func (s TxService) EncodeJSON() []byte {}

And we call CreateTx in the cmd, then string(TxService.EncodeJSON())

Can you please check if we really need an accountName param in CreateTx? I see in the code that we use it to set these before preparing the broadcast func. Looks like this has to be set anyway even if we just want to create an unsigned tx, can you please confirm that? Otherwise we can move the accountName param to TxService.Broadcast(accountName string).

Yeah, good idea. I was indeed confused by that naming, so cleaning this up would be great. Will do!

@ilgooz I did most of the work for this and there is one thing that is making accountName necessary in the current design.
It is needed for the gas simulation (it will fail without it because of sequence mismatch).

So the question becomes: should we

Pre-calculate gas in CreateTx similar to what has been done now
or

Not (if so, should we do it only when asked in Gas()?)

Then I think let's stick with the current API, option 1, where we get the accountName in CreateTx and calculate the gas beforehand. Otherwise the API will be a bit more complicated.

@ilgooz I made the changes. Take a look at the design and see if it makes sense to you.

ilgooz · 2022-06-02T10:57:47Z

ignite/pkg/cosmosclient/cosmosclient.go

-func (c Client) Status(ctx context.Context) (*ctypes.ResultStatus, error) {
-	return c.RPC.Status(ctx)
-}
+func performQuery[T any](c Client, q func() (T, error)) (T, error) {


ivanovpetr · 2022-06-03T10:34:27Z

ignite/pkg/cosmosclient/cosmosclient.go

@@ -143,6 +150,18 @@ func WithUseFaucet(faucetAddress, denom string, minAmount uint64) Option {
 	}
 }

+func WithGas(gas string) Option {


Could you please add comments to the method, same for WithGasPrices

ivanovpetr · 2022-06-03T10:49:05Z

ignite/pkg/cosmosclient/cosmosclient.go

+
+// Bech32Address returns the bech32 address, with the correct prefix, from account name or address.
+func (c Client) Bech32Address(accountNameOrAddress string) (string, error) {
+	unlockFn := c.lockBech32Prefix()


Suggested change

unlockFn := c.lockBech32Prefix()

defer c.lockBech32Prefix()()

I thought about this. I'll do it, but I just wasn't sure if this really reads very clearly (it sort of looks like we're deferring the locking).

ivanovpetr · 2022-06-03T10:49:50Z

ignite/pkg/cosmosclient/cosmosclient.go

-	return c.RPC.Status(ctx)
-}
+func performQuery[T any](c Client, q func() (T, error)) (T, error) {
+	unlockFn := c.lockBech32Prefix()


Suggested change

unlockFn := c.lockBech32Prefix()

defer c.lockBech32Prefix()()

ivanovpetr · 2022-06-03T10:50:13Z

ignite/pkg/cosmosclient/cosmosclient.go

-	defer mconf.Unlock()
-	config := sdktypes.GetConfig()
-	config.SetBech32PrefixForAccount(c.addressPrefix, c.addressPrefix+"pub")
+	unlockFn := c.lockBech32Prefix()


Suggested change

unlockFn := c.lockBech32Prefix()

defer c.lockBech32Prefix()()

ivanovpetr · 2022-06-03T11:01:32Z

ignite/cmd/node_tx_bank_send.go

+		return err
+	}
+
+	session.Println("Transaction broadcast successful!")


Suggested change

session.Println("Transaction broadcast successful!")

session.StopSpinner()

session.Println("Transaction broadcast successful!")

At this point spinner should be stopped manually

ivanovpetr · 2022-06-03T11:03:12Z

ignite/cmd/node_query_bank_balances.go

+		rows = append(rows, []string{fmt.Sprintf("%s", b.Amount), b.Denom})
+	}
+
+	return session.PrintTable([]string{"Amount", "Denom"}, rows...)


Suggested change

return session.PrintTable([]string{"Amount", "Denom"}, rows...)

session.StopSpinner()

return session.PrintTable([]string{"Amount", "Denom"}, rows...)

ilgooz · 2022-06-06T16:58:55Z

ignite/pkg/cosmosclient/txservice.go

+
+// EncodeJSON encodes the transaction as a json string
+func (s TxService) EncodeJSON() ([]byte, error) {
+	json, err := s.client.context.TxConfig.TxJSONEncoder()(s.txBuilder.GetTx())


Suggested change

json, err := s.client.context.TxConfig.TxJSONEncoder()(s.txBuilder.GetTx())

return s.client.context.TxConfig.TxJSONEncoder()(s.txBuilder.GetTx())

gjermundgaraba · 2022-06-14T18:39:32Z

@ilgooz Just to make sure, you're not waiting for me to do anything here right?

ilgooz · 2022-06-14T18:57:33Z

Hey Gjermund!

I'm refactoring some bits, will push some commits soon!

ilgooz

We should not require address prefix when querying, if the account address is used while querying

gjermundgaraba · 2022-06-14T21:19:45Z

We should not require address prefix when querying, if the account address is used while querying

I don't think we are. I just tested it and it seems to work fine without the prefix. Updating the tests to show this now.

ilgooz · 2022-06-18T08:34:15Z

@bjaanes oh okay, that comment was more like a comment to remind to myself during review, sorry for the confusion!

ilgooz · 2022-07-25T13:37:25Z

I was making some fixes and improvements on this PR and also tweaking the integration package's API.

My changes are in this branch within a50618c commit.

@jeronimoalbi @tbruyelle can you please take a look and maybe continue from here? So we can add those changes into this PR and finally merge it.

aljo242 · 2022-08-25T19:13:54Z

Can we close this?

add node command with bank query and send

55260cc

Bounty address: cosmos1zra3wz596n0qc898myceka3q23x9rk8fuw65c7

gjermundgaraba requested review from fadeev, ilgooz, lumtis, Pantani, ivanovpetr and jeronimoalbi as code owners May 30, 2022 08:55

gjermundgaraba commented May 30, 2022

View reviewed changes

gjermundgaraba changed the title ~~add node command with bank query and send~~ feat: add node command with bank query and send May 30, 2022

gjermundgaraba mentioned this pull request May 30, 2022

add node command set w/ token transfer functionality #2489

Closed

gjermundgaraba added 2 commits May 31, 2022 19:45

Merge branch 'develop' into issue-2489-node-bank-cmds

a49d2eb

Merge branch 'develop' into issue-2489-node-bank-cmds

508d867

ilgooz requested changes Jun 2, 2022

View reviewed changes

ivanovpetr reviewed Jun 3, 2022

View reviewed changes

gjermundgaraba added 2 commits June 3, 2022 17:00

Code review cleanup

c909767

Merge branch 'develop' into issue-2489-node-bank-cmds

240423b

gjermundgaraba requested review from ilgooz and ivanovpetr June 3, 2022 15:21

ilgooz reviewed Jun 6, 2022

View reviewed changes

gjermundgaraba added 2 commits June 7, 2022 21:01

Merge branch 'develop' into issue-2489-node-bank-cmds

9bd0707

Code review fix

6ac26a7

gjermundgaraba requested a review from ilgooz June 7, 2022 19:03

ilgooz added 5 commits June 10, 2022 13:59

Merge branch 'develop' into issue-2489-node-bank-cmds

38aba13

fix merge

1ca59d0

Merge branch 'develop' into issue-2489-node-bank-cmds

f908555

Merge branch 'develop' into issue-2489-node-bank-cmds

970a5cc

refactor

836f6d2

ilgooz reviewed Jun 14, 2022

View reviewed changes

Update integration test

c78de6b

ilgooz added this to the Priority milestone Jul 25, 2022

tbruyelle mentioned this pull request Jul 27, 2022

feat: introduce node command #2669

Merged

ilgooz closed this Aug 25, 2022


		const testAccountMnemonic = "develop mansion drum glow husband trophy labor jelly fault run pause inside jazz foil page injury foam oppose fruit chunk segment morning series nation"

		func TestAccount(t *testing.T) {

	func (c Client) BankBalances(address string, pagination *query.PageRequest) (sdk.Coins, error) {
	func (c Client) BankBalances(ctx context.Context, address string, pagination *query.PageRequest) (sdk.Coins, error) {

	Short: "Query for account balances by address",
	Short: "Query for account balances by account name or address",

	Use: "balances [address]",
	Use: "balances [account-name\|address]",


		commit, err := node.Commit(ctx, &height)
		func (c Client) GenerateTx(accountName string, msgs ...sdktypes.Msg) (string, error) {

	unlockFn := c.lockBech32Prefix()
	defer c.lockBech32Prefix()()

	session.Println("Transaction broadcast successful!")
	session.StopSpinner()
	session.Println("Transaction broadcast successful!")

	return session.PrintTable([]string{"Amount", "Denom"}, rows...)
	session.StopSpinner()
	return session.PrintTable([]string{"Amount", "Denom"}, rows...)

	json, err := s.client.context.TxConfig.TxJSONEncoder()(s.txBuilder.GetTx())
	return s.client.context.TxConfig.TxJSONEncoder()(s.txBuilder.GetTx())

feat: add node command with bank query and send #2539

feat: add node command with bank query and send #2539

Conversation

gjermundgaraba commented May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilgooz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanovpetr Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanovpetr Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjermundgaraba commented Jun 14, 2022

ilgooz commented Jun 14, 2022

ilgooz left a comment

Choose a reason for hiding this comment

gjermundgaraba commented Jun 14, 2022

ilgooz commented Jun 18, 2022 • edited Loading

ilgooz commented Jul 25, 2022

aljo242 commented Aug 25, 2022

gjermundgaraba commented May 30, 2022 •

edited

Loading

ivanovpetr Jun 3, 2022 •

edited

Loading

ivanovpetr Jun 3, 2022 •

edited

Loading

ilgooz commented Jun 18, 2022 •

edited

Loading