-
Notifications
You must be signed in to change notification settings - Fork 3
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
Bill Currency Utils For Conversation Support #371
base: master
Are you sure you want to change the base?
Bill Currency Utils For Conversation Support #371
Conversation
@zupzup kindly review this Draft PR, am a bit confuse on where to use it in the codebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, this looks fine with some changes (I added a few comments).
The BTC to Sat conversion is optional for now, since we don't use it anywhere as of yet.
In terms of where to add it - the validation would need to be added to every place where a sum
, or amount
with a currency
is sent to the Web API (e.g. issue bill).
} | ||
|
||
// Convert a Currency enum to a string | ||
pub fn as_str(&self, currency: &str) -> Option<&'static str> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just take &self
and can just return &'static str
, it doesn't have to return an Option, since you can cover every case (currently only Currency::Sat
).
pub fn as_str(&self) -> &'static str {
match self {
Currency::Sat => "sat",
}
}
} | ||
|
||
// From satoshis to BTC | ||
pub fn sat_to_btc(sats: u64) -> f64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using f64 for currency conversion is problematic, because floating point ops can introduce precision errors.
To to this correctly, we'd need something like rust-decimal (https://docs.rs/rust_decimal/latest/rust_decimal/) or something similar.
Since we currently send the sum
as a string, it would also be OK to convert to a BTC string like "0.100".
π Description
Please provide a brief summary of the changes in this pull request. What issue(s) does this PR address? What problem(s) does it solve?
Relates to #328
β Checklist
Please ensure the following tasks are completed before requesting a review:
cargo fmt
.cargo clippy
.π Changes Made
π‘ How to Test
Please provide clear instructions on how reviewers can test your changes:
π€ Related Issues
List any related issues, pull requests, or discussions:
π Additional Context (optional)
Add any other context or information that might be useful for reviewers:
[Additional notes or links]
π Relevant Documentation (optional)
Provide links to relevant documentation that reviewers may need:
π Review Guidelines
Please focus on the following while reviewing: