Skip to content
This repository was archived by the owner on Jan 5, 2026. It is now read-only.

Conversation

@stalniy
Copy link
Contributor

@stalniy stalniy commented Feb 27, 2025

Why

Current implementation relies on parseInt + devision which will loose precision for very long numbers

What

  1. Rewrites DecCoin encode/decode to relying on BigInt checks + string manipulation
  2. Adds more test cases to validate edge cases

@stalniy stalniy requested a review from a team as a code owner February 27, 2025 03:56
@stalniy stalniy force-pushed the fix/dec-coin-encoding branch 2 times, most recently from 582f255 to c87b9f1 Compare February 27, 2025 03:58
@stalniy stalniy force-pushed the fix/dec-coin-encoding branch 2 times, most recently from 9f1e13c to 557a9ea Compare February 27, 2025 12:06
ygrishajev
ygrishajev previously approved these changes Feb 27, 2025
Copy link
Member

@troian troian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you rebase it on to sdk-47 branch

@stalniy
Copy link
Contributor Author

stalniy commented Mar 4, 2025

just to double check, @troian are you asking to change PR target branch to be sdk-47 instead of main?

@troian
Copy link
Member

troian commented Mar 4, 2025

correct, those changes are comming to effect on sdk-47 upgrade

@stalniy
Copy link
Contributor Author

stalniy commented Mar 5, 2025

well, this is now impossible because you merged my previous PR which relies on different tool to generate types. And patching should be done in a different way. The aim of this PR was to fix the current sdk version

Anyway, thanks to @baktun14, I can talk to telescope owners/developers, so potentially they will do the fix on their side and we will not need to use patching and I will change the implementation in sdk-47 branch. So, let's wait for some time

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants