gaiacli sign --offline should require additional flags

[jackzampolin]

The --account-number, --sequence and --chain-id flags should be required.

[alexanderbez]

--chain-id is already marked required from what I can see. The account values should be required though 👍

[alessio]

We should implement a conditional logic in the CLIContext that populates account number, sequence, and all the rest according to the offline flag, i.e. if offline is false then populate values automatically (provides that they are not overridden by their respective cli flags and the node is reachable); else leave them blank and leave error handling with the caller.

[yangyanqing]

Maybe we can add new command for sign offline with sequence and account-number as argument.
The default int value 0 is legal, so we can't know whether the flags were input.

[alexanderbez]

Don't we already do this @alessio during the sign command?

@yangyanqing I think we should really avoid new commands if possible. The default value can just be -1 in this case.

[yangyanqing]

These two flag's type was Uint64, so -1 can not be set as the default value.

Function populateAccountFromState() assume 0 was not assign.

func populateAccountFromState(txBldr authtxb.TxBuilder, cliCtx context.CLIContext,
	addr sdk.AccAddress) (authtxb.TxBuilder, error) {
	if txBldr.AccountNumber() == 0 {
		accNum, err := cliCtx.GetAccountNumber(addr)
		if err != nil {
			return txBldr, err
		}
		txBldr = txBldr.WithAccountNumber(accNum)
	}

	if txBldr.Sequence() == 0 {
		accSeq, err := cliCtx.GetAccountSequence(addr)
		if err != nil {
			return txBldr, err
		}
		txBldr = txBldr.WithSequence(accSeq)
	}

	return txBldr, nil
}

But we can't make that assumptions, except:

1. Assuming account 0 is never used for sign offline (can be satisfied)
2. Assuming the first tx had be done online (can not be satisfied for offline wallet)

➜ cosmos-sdk git:(frank/3893-gaiacli-sign-offline) ✗ gaiacli query account cosmos1h7urjzmuejtu6h96t744r2euvqekar6ujupegm(validator account address)

{
  "type": "auth/Account",
  "value": {
    "address": "cosmos1h7urjzmuejtu6h96t744r2euvqekar6ujupegm",
    "coins": [
      {
        "denom": "apple",
        "amount": "99999"
      },
      {
        "denom": "stake",
        "amount": "199999900"
      }
    ],
    "public_key": {
      "type": "tendermint/PubKeySecp256k1",
      "value": "AzpJconKtox/Pf774JosZhASDDkRS5E+j4yKcPJXb0jn"
    },
    "account_number": "0",
    "sequence": "2"
  }
}

[alexanderbez]

Sure, so we can make the type Int, no?

[yangyanqing]

Issue #2701 discarded the type switching and PR #2799 switching type from Int64 to Uint64.

Choosing int sequence means reducing the number of available tx by half.

What do you think about this issue ? @sunnya97

[alexanderbez]

No no, I mean the CLI flags and TxBuilder can use ints. The account must use unsigned ints!

[yangyanqing]

No no, I mean the CLI flags and TxBuilder can use ints. The account must use unsigned ints!

Choosing int sequence means reducing the number of available tx by half.
It means user can not send by cli after half of the capacity.
But 10^63 is large enough to one account, so I want to know @sunnya97 's idea, who had switch type from int64 to uint64

[alexanderbez]

Ahh I suppose you're right. The reason was collective. No reason they should be signed integers. For offline mode we can perhaps check via Viper if the flag was set/changed.

[yangyanqing]

Ahh I suppose you're right. The reason was collective. No reason they should be signed integers. For offline mode we can perhaps check via Viper if the flag was set/changed.

Great idea! I'll try it.

[alessio]

Sure, so we can make the type Int, no?

Or we can turn the CLI flag into a string, and parse it as Uint later on.

[jackzampolin]

As a side note, these flags should be -a --account-number -s --sequence -o --offline

[alessio]

I agree @jackzampolin, shorthands for common and important flags should always be provided.