-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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!: remove legacyamino functions #17746
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.
Pretty substantial PR, but from the looks of it, it seems good to me.
@@ -58,24 +58,6 @@ service Service { | |||
body: "*" | |||
}; | |||
} | |||
// TxEncodeAmino encodes an Amino transaction from JSON to encoded bytes. |
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.
Why do we delete this actually?
Wasn't it added per community request?
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 think I agree; we can still encode to amino JSON via the encoder in x/tx or is this talking about a different amino format? (I remember something about a binary amino format).
Instead maybe keep the messages/services and have the decode service return an error since this is what we're dropping support for.
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.
technically this doesn't work for modules that dont registerlegacyamino, so we have a bit of a discrepancy on support here. I can look at adding back the encode but decode id just remove.
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.
Shouldn't this stay as well? cc @kocubinski
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 really think we should find a way to keep these tests; even if that means moving the (now) legacy amino json encoder dependency into a _test
namespace or package.
They are asserting that the amino json encoder has equivalent output to the legacy one which could prove valuable if issues arise down the road.
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 can look at registering the amino types needed for this test in the test but the modules wont be reverted. Modules need to move forward and breaking things is part of it. Otherwise we will never be able to move forward
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 see what you mean. I'm not sure how this would work right exactly right now, but another possibility is to obtain a "golden file" comprising the output of passing the proto types generated through rapidproto at a particular seed (doesn't matter what).
Since this golden file, made in good faith, has the same output as legacy amino json encoder we could delete the encoder code and all the wiring for it but still assert the correctness of the x/tx one.
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.
Also the golden file is committed to source control.
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 think we should retain the generative tests, lmk if you need help generating the golden file.
closing this for now, would like to pair it with another issue |
Description
second attempt at removing amino usage in the sdk in favor of the automated version
This pr removes:
ref #17775
Author 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
make lint
andmake test
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