amino signing for gov deposit transactions fails

[clevinson]

amino signing for gov deposit transactions fails

[aleem1314]

@aleem1314 can you confirm that you think this resolves the issue? Were there other amino tests that were failing besides group/MsgDeposit that aren't explained by the uint64 => string conversion?

Yes. Converting proposalId to string fixes the issue. Both Deposit and Vote messages working with ledger.

[clevinson]

@ryanchristo and I have both reproduced the issue.

@ryanchristo using the repo above (and this hackMD: https://hackmd.io/Q-iN8S0sS-afyRpt8np6Xg)

I reproduced using the CLI (and attempting to manually inspect the sign bytes on my ledger device). See below:

$ regen tx gov deposit 1 5000uregen --fees 5000uregen --from ledger_test
# Default sign-mode 'direct' not supported by Ledger, using sign-mode 'amino-json'.
# {"body":{"messages":[{"@type":"/cosmos.gov.v1.MsgDeposit","proposal_id":"1","depositor":"regen1mrvlgpmrjn9s7r7ct69euqfgxjazjt2l5lzqcd","amount":[{"denom":"uregen","amount":"5000"}]}],"memo":"","timeout_height":"0","extension_options":[],"non_critical_extension_options":[]},"auth_info":{"signer_infos":[],"fee":{"amount":[{"denom":"uregen","amount":"5000"}],"gas_limit":"200000","payer":"","granter":""},"tip":null},"signatures":[]}
# confirm transaction before signing and broadcasting [y/N]: y
# Error: JSON Contains whitespace in the corpus

regen tx gov submit-proposal works fine, fwiw.

[ryanchristo]

Not sure what's going on with your attempt but I was able to successfully submit a deposit via the cli using a ledger device.

./build/regen tx gov deposit 1 5000uregen --fees 5000uregen --from ledger_test

  total_deposit:
  - amount: "5000"
    denom: uregen

[ryanchristo]

Maybe regen needed to be ./build/regen and you were using another version...?

[ryanchristo]

This leaves me thinking that either there is an issue in Keplr with the new gov messages or in the testing repository.

[ryanchristo]

One thing I noticed is that the gov module is the only module that registers amino messages using a version in the name:

	legacy.RegisterAminoMsg(cdc, &MsgDeposit{}, "cosmos-sdk/v1/MsgDeposit")

I also noticed the group module has inconsistent amino message names (some with /group and some without):

	legacy.RegisterAminoMsg(cdc, &MsgCreateGroup{}, "cosmos-sdk/MsgCreateGroup")
	legacy.RegisterAminoMsg(cdc, &MsgLeaveGroup{}, "cosmos-sdk/group/MsgLeaveGroup")

I updated the test repository to test MsgLeaveGroup and received the same error as gov MsgDeposit.

[ryanchristo]

cc @clevinson here's a fork with MsgLeaveGroup: https://github.com/ryanchristo/group-gov-amino-testing

[ryanchristo]

Here's a working version of MsgLeaveGroup: https://github.com/ryanchristo/group-gov-amino-testing/commit/a5d2164c236e2bb72680accc2902b52181196049

[clevinson]

So I'm not sure why my CLI tx's were failing above, but they seem to be working now. So let's ignore my previous bug report involving "Error: JSON Contains whitespace in the corpus".

To further debug, I put some fmt.PrintF's in the regen ledger amino verification pathway, and was able to verify the difference btw what @aleem1314's repo generates, and what the server expects.

AminoConverter generates:
{"chain_id":"regen-sandbox","account_number":"11","sequence":"3","fee":{"amount":[{"amount":"2600000","denom":"uregen"}],"gas":"260000"},"msgs":[{"type":"cosmos-sdk/v1/MsgDeposit","value":{"amount":[{"denom":"uregen","amount":"1000"}],"depositor":"regen1mrvlgpmrjn9s7r7ct69euqfgxjazjt2l5lzqcd","proposal_id":1}}],"memo":""}

Server expects (similar tx sent over CLI):
{"account_number":"11","chain_id":"regen-sandbox","fee":{"amount":[{"amount":"5000","denom":"uregen"}],"gas":"200000"},"memo":"","msgs":[{"type":"cosmos-sdk/v1/MsgDeposit","value":{"amount":[{"amount":"5000","denom":"uregen"}],"depositor":"regen1mrvlgpmrjn9s7r7ct69euqfgxjazjt2l5lzqcd","proposal_id":"2"}}],"sequence":"3"}

The main difference causing the issue is that the proposal_id needs to be converted to a string in the MsgDeposit AminoConverter.

I can't find exactly where there is amino converter documentation explaining this, but I was able to verify that in regen-js, we similarly convert fields to string if they are defined as uint64 in the proto files (see here).

If the above hypothesis is correct, then it seems the bug is actually just a mis-implementation of the amino converters in the testing repo.

@aleem1314 can you confirm that you think this resolves the issue? Were there other amino tests that were failing besides group/MsgDeposit that aren't explained by the uint64 => string conversion?