Skip to content
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

update all GetSignBytes functions with correct codec #1481

Closed
4 tasks
technicallyty opened this issue Sep 7, 2022 · 4 comments · Fixed by #1480
Closed
4 tasks

update all GetSignBytes functions with correct codec #1481

technicallyty opened this issue Sep 7, 2022 · 4 comments · Fixed by #1480
Assignees
Labels
Type: Bug Something isn't working

Comments

@technicallyty
Copy link
Contributor

Summary of Bug

our GetSignBytes methods on all messages are currently producing incorrect sign bytes. The fix for MsgSend is in this PR

Version

main

Steps to Reproduce

  • call GetSignBytes of any regen ledger message
  • convert to string and print
  • the result should have type and value fields, but currently does not

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@clevinson
Copy link
Member

clevinson commented Sep 16, 2022

The short-term workaround we had anticipated for this problem (by forking CosmJS) will not actually work, since the serialization of a signDoc happens inside of the Keplr wallet extension.

As a result, we'll need to go back to our original plan to do a network upgrade & software governance proposal to get hardware wallet support working w/ the upcoming marketplace launch.

I spent some time today doing manually testing against #1480 and #1481. Unfortunately neither of them seem to work as expected.

I added a println to the bottom of GetSignBytes() in x/auth/tx/legacy_amino_json.go and was able to verify that the messages still get serialized without the necessary {"type": "....", "value": "..."} envelope.

$ regen version
main-00829dac260fa304760e399561f89eed15350a11

$ regen tx ecocredit send 500 C01-001-20200101-20210101-001 regen1df675r9vnf7pdedn4sf26svdsem3ugavgxmy46 --from cory_ledger --output json
Default sign-mode 'direct' not supported by Ledger, using sign-mode 'amino-json'.

# this is the normal proto-json output displayed to the user
{"body":{"messages":[{"@type":"/regen.ecocredit.v1.MsgSend","sender":"regen10sc6nxu0uc4ss5m4cxdzhwcahc9zte9allwrsh","recipient":"regen1df675r9vnf7pdedn4sf26svdsem3ugavgxmy46","credits":[{"batch_denom":"C01-001-20200101-20210101-001","tradable_amount":"500","retired_amount":"0","retirement_jurisdiction":""}]}],"memo":"","timeout_height":"0","extension_options":[],"non_critical_extension_options":[]},"auth_info":{"signer_infos":[],"fee":{"amount":[],"gas_limit":"200000","payer":"","granter":""},"tip":null},"signatures":[]}
confirm transaction before signing and broadcasting [y/N]: y

# this is the amino GetSignBytes() output from `x/auth/tx/legacy_amino_json.go`
Serialized SignDoc: {"account_number":"103","chain_id":"regen-redwood-1","fee":{"amount":[],"gas":"200000"},"memo":"","msgs":[{"credits":[{"batch_denom":"C01-001-20200101-20210101-001","retired_amount":"0","tradable_amount":"500"}],"recipient":"regen1df675r9vnf7pdedn4sf26svdsem3ugavgxmy46","sender":"regen10sc6nxu0uc4ss5m4cxdzhwcahc9zte9allwrsh"}],"sequence":"4"}

@AmauryM @technicallyty do either of you have any idea what may be missing here? I know Amaury had written a unit test to verify that the serialization was working correct... Are there different codecs being wired up at some other point that don't make use of the ones from #1480 ?

@clevinson
Copy link
Member

clevinson commented Sep 16, 2022

For reference, here's what sending a normal bank/MsgSend tx looks like:

$ regen tx bank send $(regen keys show -a cory_ledger) regen1df675r9vnf7pdedn4sf26svdsem3ugavgxmy46 5000uregen --from cory_ledger --output json
Default sign-mode 'direct' not supported by Ledger, using sign-mode 'amino-json'.

# this is the normal proto-json output displayed to the user
{"body":{"messages":[{"@type":"/cosmos.bank.v1beta1.MsgSend","from_address":"regen10sc6nxu0uc4ss5m4cxdzhwcahc9zte9allwrsh","to_address":"regen1df675r9vnf7pdedn4sf26svdsem3ugavgxmy46","amount":[{"denom":"uregen","amount":"5000"}]}],"memo":"","timeout_height":"0","extension_options":[],"non_critical_extension_options":[]},"auth_info":{"signer_infos":[],"fee":{"amount":[],"gas_limit":"200000","payer":"","granter":""},"tip":null},"signatures":[]}
confirm transaction before signing and broadcasting [y/N]: y

# this is the amino GetSignBytes() output from `x/auth/tx/legacy_amino_json.go`
Serialized SignDoc: {"account_number":"103","chain_id":"regen-redwood-1","fee":{"amount":[],"gas":"200000"},"memo":"","msgs":[{"type":"cosmos-sdk/MsgSend","value":{"amount":[{"amount":"5000","denom":"uregen"}],"from_address":"regen10sc6nxu0uc4ss5m4cxdzhwcahc9zte9allwrsh","to_address":"regen1df675r9vnf7pdedn4sf26svdsem3ugavgxmy46"}}],"sequence":"4"}

@ryanchristo ryanchristo added the Type: Bug Something isn't working label Sep 16, 2022
@technicallyty
Copy link
Contributor Author

i'm getting x/auth/migrations/legacytx/stdsign.go's method StdSignBytes returning the following after this call

bz, err := legacy.Cdc.MarshalJSON(StdSignDoc{
		AccountNumber: accnum,
		ChainID:       chainID,
		Fee:           json.RawMessage(fee.Bytes()),
		Memo:          memo,
		Msgs:          msgsBytes,
		Sequence:      sequence,
		TimeoutHeight: timeout,
		Tip:           stdTip,
	})
	if err != nil {
		panic(err)
	}

resulting in bz = :

{
    "account_number": "0",
    "sequence": "9",
    "chain_id": "chain-ysrrFL",
    "memo": "",
    "fee": {
        "amount": [
            {
                "denom": "stake",
                "amount": "10"
            }
        ],
        "gas": "200000"
    },
    "msgs": [
        {
            "type": "regen/MsgCreateClass",
            "value": {
                "admin": "regen1znmkg3qgvvz3f3qxnfw628kd5e7ueseschfsc3",
                "credit_type_abbrev": "C",
                "fee": {
                    "amount": "20000000",
                    "denom": "stake"
                },
                "issuers": [
                    "regen1znmkg3qgvvz3f3qxnfw628kd5e7ueseschfsc3"
                ],
                "metadata": "metadata"
            }
        }
    ]
}

@clevinson
Copy link
Member

Turns out this was correctly fixed on main branch, and the only issue on @technicallyty's PR was that there was no replace directive in the go.mod for x/ecocredit so it was pulling in the ecocredit v2.1 tag instead of going off of that branch's implementation.

As long as we ensure #1500 is taken care of after merging #1483, then we should be good to go.

Closing this issue in favor of #1500 now.

clevinson pushed a commit that referenced this issue Sep 27, 2022
…) (#1483)

* fix(x/ecocredit): amino registration

* chore: changelog entry

Co-authored-by: tyler <{ID}+{username}@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants