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.

Sign up for GitHub

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

Jump to bottom

Expect larger integers on blockchain.scripthash.get_balance #1218

Merged
merged 2 commits into from
Feb 25, 2022
Merged

Expect larger integers on blockchain.scripthash.get_balance #1218

merged 2 commits into from
Feb 25, 2022

Conversation

sergeyboyko0791
Copy link

Fixes #1196

@sergeyboyko0791 sergeyboyko0791 self-assigned this Feb 21, 2022
@sergeyboyko0791 sergeyboyko0791 linked an issue Feb 21, 2022 that may be closed by this pull request
invalid value, expected i64 error on blockchain.scripthash.get_balance #1196
Closed
@artemii235
Copy link
Member

artemii235 commented Feb 21, 2022 •
edited
Loading

@sergeyboyko0791 Error(\"invalid type: integer 758902000, expected a tuple of size 2\", line: 0, column: 0) from the WASM build and test Debug doesn't look like unstable test. Please ensure that all tests pass before requesting the review.

@sergeyboyko0791 sergeyboyko0791 marked this pull request as draft February 21, 2022 08:17
@sergeyboyko0791
Copy link
Author

My bad. I really believed that BigInt serializes and deserializes as BigDecimal.
I hope i128 is enough for the blockchain.scripthash.get_balance result.

@sergeyboyko0791 sergeyboyko0791 marked this pull request as ready for review February 21, 2022 11:53
artemii235
artemii235 previously approved these changes Feb 22, 2022
View reviewed changes
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

👍 @cipig could you test it, please?

@cipig
Copy link
Member

cipig commented Feb 22, 2022

+1 @cipig could you test it, please?

I am running this branch on all my nodes and on Desktop, but i don't have so many FJC any more to reproduce.
How many FJC would i need so that i64 is not enough?

@sergeyboyko0791
Copy link
Author

sergeyboyko0791 commented Feb 23, 2022 •
edited
Loading

How many FJC would i need so that i64 is not enough?

The range of i64 is [-9223372036854775808, 9223372036854775807].
Either unconfirmed < -9223372036854775808 (unconfirmed transactions are just spending outputs) or confirmed/unconfirmed > 9223372036854775807

artemii235
artemii235 approved these changes Feb 25, 2022
View reviewed changes
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

I guess we can reopen the issue/PR if the problem occurs again.

@artemii235 artemii235 merged commit 214c1df into dev Feb 25, 2022
@artemii235 artemii235 deleted the fix-electrum-balance branch February 25, 2022 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artemii235 artemii235 artemii235 approved these changes

@cipig cipig Awaiting requested review from cipig

Assignees

@sergeyboyko0791 sergeyboyko0791

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

invalid value, expected i64 error on blockchain.scripthash.get_balance
3 participants
@sergeyboyko0791 @artemii235 @cipig