-
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
feat: x/tx/signing/aminojson: Marshal sort fields #16254
Conversation
69494b9
to
1f4e97c
Compare
Kindly cc-ing @elias-orijtech @ValarDragon |
09eadd3
to
9294068
Compare
19f9be6
to
bbb8189
Compare
Can we change sdk.MustSortJson to use this then? |
There should be no more need for MustSortJson. See #16062 |
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
7460420
to
28b3a52
Compare
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, seems some integration tests are still failing
Thanks @tac0turtle and @aaronc for the reviews and approvals. Sure let me take a look at the tests. |
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.
LGTM, just one nit.
merged matts pr that fixes tests, @odeke-em if you can rebase then we can merge this |
This change gives (aminojson.Encoder).Marshal the ability to sort field names before marshalling, mimicking the outward behavior of encoding/json.Marshal and thus eliminating a long prior roundtrip that required sdk.*SortJSON which would take the produced JSON and encoding/json.Unmarshal it to an interface firstly, then encoding/json.Marshal it out to get the field names sorted. While here, this change adds an opt-out field to aminojson.EncoderOptions called "DoNotSortFields", in case one wants to preserve legacy behavior for compatibility checks/reasons. The performance benchmarks from before and after reveal improvements ```shell $ benchstat before.txt after.txt name old time/op new time/op delta AminoJSON-8 76.4µs ± 1% 61.7µs ± 2% -19.28% (p=0.000 n=9+10) name old alloc/op new alloc/op delta AminoJSON-8 15.0kB ± 0% 9.4kB ± 0% -37.28% (p=0.000 n=10+10) name old allocs/op new allocs/op delta AminoJSON-8 332 ± 0% 234 ± 0% -29.52% (p=0.000 n=10+10) ``` Fixes #2350
085ee2b
to
bf4e8fe
Compare
Awesome and thank you @kocubinski and @tac0turtle, thank you for the reviews! Rebase done. |
This change gives (aminojson.Encoder).Marshal the ability to sort field names before marshalling, mimicking the outward behavior of encoding/json.Marshal and thus eliminating a long prior roundtrip that required sdk.*SortJSON which would take the produced JSON and encoding/json.Unmarshal it to an interface firstly, then encoding/json.Marshal it out to get the field names sorted. While here, this change adds an opt-out field to aminojson.EncoderOptions called "DoNotSortFields", in case one wants to preserve legacy behavior for compatibility checks/reasons.
The performance benchmarks from before and after reveal improvements
Fixes #2350