cosmology-tech / telescope

A TypeScript Transpiler for Cosmos Protobufs ⚛️
https://cosmology.zone/products/telescope
Apache License 2.0
144 stars 43 forks source link

Fix: to amino methods #558

Closed rasoulMrz closed 5 months ago

rasoulMrz commented 8 months ago

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 commented 8 months ago

Hi, Thank you very much for your PR!

Recently I also tried to handle empty fields of amino codecs: https://github.com/cosmology-tech/telescope/pull/537 to fix this issue: https://github.com/cosmology-tech/telescope/issues/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:

https://github.com/cosmology-tech/telescope/blob/4ee02719f516fc2575a37f611ed0e1a5b1a74422/__fixtures__/osmojs/cosmos-sdk/proto/amino/amino.proto#L60-L78

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

Zetazzz commented 8 months ago

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 commented 8 months ago

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:

https://github.com/cosmology-tech/telescope/blob/4ee02719f516fc2575a37f611ed0e1a5b1a74422/__fixtures__/osmojs/cosmos-sdk/proto/amino/amino.proto#L60-L78

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 commented 8 months ago

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: https://github.com/cosmology-tech/telescope/blob/4ee02719f516fc2575a37f611ed0e1a5b1a74422/__fixtures__/osmojs/cosmos-sdk/proto/amino/amino.proto#L60-L78

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 commented 7 months ago

Hi, I created another PR for padDecimal only, and it's merged: https://github.com/cosmology-tech/telescope/pull/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 commented 7 months ago

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 commented 7 months ago

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: https://github.com/cosmology-tech/telescope/pull/569

Zetazzz commented 5 months ago

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