Skip to content

Fix getBtcBalance wrapper response#5

Merged
zoedberg merged 1 commit intoRGB-Tools:masterfrom
Pearl-Browser:fix/get-btc-balance
Dec 12, 2024
Merged

Fix getBtcBalance wrapper response#5
zoedberg merged 1 commit intoRGB-Tools:masterfrom
Pearl-Browser:fix/get-btc-balance

Conversation

@tmalahie
Copy link
Copy Markdown
Contributor

@tmalahie tmalahie commented Dec 7, 2024

The wallet.getBtcBalance method returns the data as a string instead of an object. All similar methods do a JSON.parse before to return an object.
This PR resolves the inconsistency by adding the missing parse

@zoedberg
Copy link
Copy Markdown
Member

zoedberg commented Dec 9, 2024

As we discussed in #2, node.js suffers float rounding errors, therefore we decided to get and return all numbers as strings. Then we suggest that users parse those strings with some specific library, as big.js. Therefore I'm closing this PR

@zoedberg zoedberg closed this Dec 9, 2024
@zoedberg
Copy link
Copy Markdown
Member

zoedberg commented Dec 9, 2024

Oops, sorry, my mistake. I forgot that the get_btc_balance returns an object and not a single amount. I'm afraid though that the parsing will not convert the numbers to strings. So we should implement something like we did for getting the numbers as strings (i.e. #[serde(deserialize_with = "from_str_or_number_optional")] and #[serde(deserialize_with = "from_str_or_number_mandatory")]). Therefore I'll reopen this PR

@zoedberg zoedberg reopened this Dec 9, 2024
Copy link
Copy Markdown
Member

@nicbus nicbus left a comment

Choose a reason for hiding this comment

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

a couple points:

  • please format the code
  • in example.js, please call JSON.stringify on btcBalance so it gets properly displayed

Copy link
Copy Markdown
Member

@zoedberg zoedberg left a comment

Choose a reason for hiding this comment

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

The changes look good, thanks. Please squash the commits into a single signed one, then I'll merge this

@tmalahie tmalahie force-pushed the fix/get-btc-balance branch from ee5dd31 to d0bbcfb Compare December 11, 2024 19:47
Copy link
Copy Markdown
Member

@zoedberg zoedberg left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@zoedberg zoedberg merged commit cce2706 into RGB-Tools:master Dec 12, 2024
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.

3 participants