chore: remove GetSignBytes from 29-fee and transfer msgs #4570
Conversation
damiannolan
requested review from
AdityaSripal,
charleenfei and
colin-axner
as code owners
|
So from the issue its not clear to me exactly what we are doing wrt proto annotations. Should we add |
There was a problem hiding this comment.
Thanks @damiannolan! It might be nice if we do a manual test with a ledger before final release
…-bytes' into damian/3911-rm-sign-bytes
| @@ -25,6 +26,7 @@ service Msg { | |||
| // ICS20 enabled chains. See ICS Spec here: | |||
| // https://github.com/cosmos/ibc/tree/master/spec/app/ics-020-fungible-token-transfer#data-structures | |||
| message MsgTransfer { | |||
| option (amino.name) = "cosmos-sdk/MsgTransfer"; | |||
There was a problem hiding this comment.
As far as I understand, these names are for backwards compatibility and must match that which is used in RegisterConcrete legacy amino cdc.
I'm unsure if any other annotations are required here.
There was a problem hiding this comment.
here is a good example: https://github.com/cosmos/cosmos-sdk/blob/44e90e061effe955ad52d4b191897bdff41426a8/proto/cosmos/bank/v1beta1/tx.proto#L38-L54
i am working on adding docs on this but bogged down with many things.
on coin you may need to add the extra notations. Id recommmend some golden tests to make sure they arent breaking. If they are breaking then it could break some frontends based on how they are written
There was a problem hiding this comment.
Thank you! docs would be great, but no rush on that! This is great and I can try to do some testing with a ledger.
I saw this usage of legacy_coins alright, but I wasn't sure what exactly this string does, seems kind of magic! is there somewhere in the go which ties things together? I think this will be needed for coins on MsgTransfer, will add that and test things out!
# Conflicts: # modules/apps/29-fee/types/fee.pb.go # modules/apps/29-fee/types/tx.pb.go # modules/apps/transfer/types/tx.pb.go
|
I was planning on testing this PR with a ledger before merging 😅 |
Description
Removes
GetSignBytesmethod from msgs in29-feeandtransfercloses: #3911
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.