-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
refactor(codec)!: update codec to x/tx v0.6.0 #15873
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
@@ -26,15 +26,15 @@ type ( | |||
// GetMsgAnySigners returns the signers of the given message encoded in a protobuf Any | |||
// as well as the decoded google.golang.org/protobuf/proto.Message that was used to | |||
// extract the signers so that this can be used in other contexts. | |||
GetMsgAnySigners(msg *types.Any) ([]string, protov2.Message, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does changing this interface contract require a mention in the CHANGELOG?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's new anyway and there are CHANGELOG entries already for this addition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General approach looks good, but it seems there are test failures whose cause isn't obvious to me from first read, will need a deeper look to understand if there is a fundamental problem with this approach or (hopefully) if they are trivial to address.
x/auth/tx/config/config.go
Outdated
// - Provide static prefix there exported from config? | ||
// - Just do as below? | ||
AddressCodec: authcodec.NewBech32Codec(sdkConfig.GetBech32AccountAddrPrefix()), | ||
ValidatorCodec: authcodec.NewBech32Codec(sdkConfig.GetBech32ValidatorAddrPrefix()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like some feedback re the above comments. What approach do you think is best?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to find an alternative way to do this that pulls the codecs from the depinject container. I think it's fine for now as I did something similar and I intend to address in a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple minor nits, nothing blocking a merge. I don't have the full picture in my head on this, but all the changes in the PR seem fine.
Co-authored-by: Matt Kocubinski <mkocubinski@gmail.com>
Description
Ref: #11275
Pulls out the
codec
updates from #15284 into a smaller PRAuthor Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change