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

Fix: to amino methods #558

Merged
merged 16 commits into from
Mar 18, 2024
Merged

Fix: to amino methods #558

merged 16 commits into from
Mar 18, 2024

Conversation

rasoulMrz
Copy link
Collaborator

This PR focuses on fixing issues with amino encoding, mainly changing the way default values are handled with respect to omit_empty rules.
At top of this, It also fixes the issue with decimal values by automatically padding the decimal strings to 18 decimal points.

@Zetazzz
Copy link
Collaborator

Zetazzz commented Jan 6, 2024

Hi, Thank you very much for your PR!

Recently I also tried to handle empty fields of amino codecs:
#537
to fix this issue:
#522

And here’s the tests:
https://github.com/cosmology-tech/telescope/blob/25d5b5de85633ec00432c66e03977fe9a0f008f5/__fixtures__/misc/output/misc/eval_request.ts#L1936C5-L1936C5

Based on the document, I believe for aminoJSON, we should use dont_omitempty to decide how we omit:

// dont_omitempty sets the field in the JSON object even if
// its value is empty, i.e. equal to the Golang zero value. To learn what
// the zero values are, see https://go.dev/ref/spec#The_zero_value.
//
// Fields default to `omitempty`, which is the default behavior when this
// annotation is unset. When set to true, then the field value in the
// JSON object will be set, i.e. not `undefined`.
//
// Example:
//
// message Foo {
// string bar = 1;
// string baz = 2 [(amino.dont_omitempty) = true];
// }
//
// f := Foo{};
// out := AminoJSONEncoder(&f);
// out == {"baz":""}
bool dont_omitempty = 11110005;

And for (gogoproto.jsontag) = "omitempty", is it possible that it’s for to and fromJson?

@Zetazzz
Copy link
Collaborator

Zetazzz commented Jan 6, 2024

And could you please specify other bugs you're trying to fix with a few lines(better post another PR without omitempty issue) , then we can see if we can merge them first beside the omitempty issue. That would be so much appreciated!

@rasoulMrz
Copy link
Collaborator Author

Hi, Thank you very much for your PR!

Recently I also tried to handle empty fields of amino codecs: #537 to fix this issue: #522

And here’s the tests: https://github.com/cosmology-tech/telescope/blob/25d5b5de85633ec00432c66e03977fe9a0f008f5/__fixtures__/misc/output/misc/eval_request.ts#L1936C5-L1936C5

Based on the document, I believe for aminoJSON, we should use dont_omitempty to decide how we omit:

// dont_omitempty sets the field in the JSON object even if
// its value is empty, i.e. equal to the Golang zero value. To learn what
// the zero values are, see https://go.dev/ref/spec#The_zero_value.
//
// Fields default to `omitempty`, which is the default behavior when this
// annotation is unset. When set to true, then the field value in the
// JSON object will be set, i.e. not `undefined`.
//
// Example:
//
// message Foo {
// string bar = 1;
// string baz = 2 [(amino.dont_omitempty) = true];
// }
//
// f := Foo{};
// out := AminoJSONEncoder(&f);
// out == {"baz":""}
bool dont_omitempty = 11110005;

And for (gogoproto.jsontag) = "omitempty", is it possible that it’s for to and fromJson?

Thanks, The main issue with this is that as far as I have tested, when I generate go codes from protos, it ignores the dont_omitempty tag! So first main thing is to use the jsongtags instead! Second, Most of the cases the chain does omit empties, so my first priority was to fix the library so it omits the empty values before signing amino.

@Zetazzz
Copy link
Collaborator

Zetazzz commented Jan 10, 2024

Hi, Thank you very much for your PR!
Recently I also tried to handle empty fields of amino codecs: #537 to fix this issue: #522
And here’s the tests: https://github.com/cosmology-tech/telescope/blob/25d5b5de85633ec00432c66e03977fe9a0f008f5/__fixtures__/misc/output/misc/eval_request.ts#L1936C5-L1936C5
Based on the document, I believe for aminoJSON, we should use dont_omitempty to decide how we omit:

// dont_omitempty sets the field in the JSON object even if
// its value is empty, i.e. equal to the Golang zero value. To learn what
// the zero values are, see https://go.dev/ref/spec#The_zero_value.
//
// Fields default to `omitempty`, which is the default behavior when this
// annotation is unset. When set to true, then the field value in the
// JSON object will be set, i.e. not `undefined`.
//
// Example:
//
// message Foo {
// string bar = 1;
// string baz = 2 [(amino.dont_omitempty) = true];
// }
//
// f := Foo{};
// out := AminoJSONEncoder(&f);
// out == {"baz":""}
bool dont_omitempty = 11110005;

And for (gogoproto.jsontag) = "omitempty", is it possible that it’s for to and fromJson?

Thanks, The main issue with this is that as far as I have tested, when I generate go codes from protos, it ignores the dont_omitempty tag! So first main thing is to use the jsongtags instead! Second, Most of the cases the chain does omit empties, so my first priority was to fix the library so it omits the empty values before signing amino.

Ok, I got it. I'll see what I can do asap to get these feature merged

@Zetazzz
Copy link
Collaborator

Zetazzz commented Jan 16, 2024

Hi, I created another PR for padDecimal only, and it's merged:
#564

I added more test and an option for padDecimal "aminoEncoding.customTypes.useCosmosSDKDec". You can use this option to enable the feature.

And I also noticed that Dec arrays are not handled, so I refined the logic in the PR.

@rasoulMrz
Copy link
Collaborator Author

Hi, I created another PR for padDecimal only, and it's merged: #564

I added more test and an option for padDecimal "aminoEncoding.customTypes.useCosmosSDKDec". You can use this option to enable the feature.

And I also noticed that Dec arrays are not handled, so I refined the logic in the PR.

Thanks, this helps alot.
What should we do about the other things, like the omit_empty issue?

@Zetazzz
Copy link
Collaborator

Zetazzz commented Jan 16, 2024

Hi, I created another PR for padDecimal only, and it's merged: #564
I added more test and an option for padDecimal "aminoEncoding.customTypes.useCosmosSDKDec". You can use this option to enable the feature.
And I also noticed that Dec arrays are not handled, so I refined the logic in the PR.

Thanks, this helps alot. What should we do about the other things, like the omit_empty issue?

Yes, I'm on it, created another PR for it:
#569

@Zetazzz Zetazzz merged commit 5da1cad into cosmology-tech:main Mar 18, 2024
1 check failed
@Zetazzz
Copy link
Collaborator

Zetazzz commented Mar 18, 2024

Merged, thx to your contribution. Next time please make sure to commit one bug fix each PR instead of mixing them up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants