Improving currencyrate plugin #9011
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,10 @@ mod oracle; | |
| const DEFAULT_PROXY_PORT: u16 = 9050; | ||
| const MSAT_PER_BTC: f64 = 1e11; | ||
|
|
||
| fn round_to_3dp(price: f64) -> f64 { | ||
| format!("{:.3}", price).parse::<f64>().unwrap_or(price) | ||
|
Collaborator
There was a problem hiding this comment. Instead of moving from f64 to a string and then back again to f64, it would be better to do some purely mathematical operation.
Collaborator
There was a problem hiding this comment. to always get 3 decimal places you must return a String: fn round_to_3dp(price: f64) -> String {
format!("{:.3}", (price * 1000.0).round() / 1000.0)
}
Collaborator
Author
There was a problem hiding this comment. This method is not that reliable. See here the comment for very large number. That's why I opted for the f64 --> String --> f64 solution. I need to return again f64 since the price is used to compute the median, so it is better to return f64 again in my opinion.
Collaborator
There was a problem hiding this comment. Oh, the linked issue said to always have 3 decimal places not at most 3. So this is fine then if we can live with always rounding toward 0. |
||
| } | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct SourceResult { | ||
| pub name: String, | ||
|
|
@@ -104,6 +108,12 @@ median from currencyrates results", | |
| async fn currencyconvert(plugin: Plugin<PluginState>, args: Value) -> Result<Value, anyhow::Error> { | ||
| let (amount, currency) = match args { | ||
| Value::Array(values) => { | ||
| if values.len() > 2 { | ||
| return Err(anyhow!( | ||
| "Too many arguments: expected at most 2 (amount currency), got {}", | ||
|
Collaborator
There was a problem hiding this comment. nit: change this to |
||
| values.len() | ||
| )); | ||
| } | ||
| let amount = values | ||
| .first() | ||
| .ok_or_else(|| anyhow!("Missing amount"))? | ||
|
|
@@ -146,42 +156,62 @@ async fn currencyconvert(plugin: Plugin<PluginState>, args: Value) -> Result<Val | |
| } | ||
|
|
||
| async fn currencyrate(plugin: Plugin<PluginState>, args: Value) -> Result<Value, anyhow::Error> { | ||
| let (currency, source) = match args { | ||
| Value::Array(values) => { | ||
| if values.len() > 2 { | ||
| return Err(anyhow!( | ||
| "Too many arguments: expected at most 2 (currency [source]), got {}", | ||
| values.len() | ||
| )); | ||
| } | ||
| let currency = values | ||
| .first() | ||
| .ok_or_else(|| anyhow!("Missing currency"))? | ||
| .as_str() | ||
| .ok_or_else(|| anyhow!("currency must be a string"))? | ||
| .to_uppercase(); | ||
| let source = values.get(1).and_then(|v| v.as_str()).map(str::to_owned); | ||
|
Collaborator
There was a problem hiding this comment. We do allow sources to be named as numbers only and this would then cause currencyrate to silently ignore the sourrce and return a median result. I suggest changing this to: let source = values.get(1).and_then(|v| {
v.as_str()
.map(str::to_owned)
.or_else(|| v.as_number().map(std::string::ToString::to_string))
}); |
||
| (currency, source) | ||
| } | ||
| Value::Object(map) => { | ||
|
Collaborator
There was a problem hiding this comment. We should then also do the same check for object based queries here aswell. |
||
| let currency = map | ||
| .get("currency") | ||
| .ok_or_else(|| anyhow!("Missing currency"))? | ||
| .as_str() | ||
| .ok_or_else(|| anyhow!("currency must be a string"))? | ||
| .to_uppercase(); | ||
| let source = map.get("source").and_then(|v| v.as_str()).map(str::to_owned); | ||
|
Collaborator
There was a problem hiding this comment. Same as above: let source = map.get("source").and_then(|v| {
v.as_str()
.map(str::to_owned)
.or_else(|| v.as_number().map(std::string::ToString::to_string))
}); |
||
| (currency, source) | ||
| } | ||
| _ => return Err(anyhow!("Arguments must be an array or dictionary")), | ||
| }; | ||
|
|
||
| let oracle = plugin.state().oracle.lock().await; | ||
| oracle.currency_requested(¤cy).await; | ||
|
|
||
| let rate = match source { | ||
| Some(source_name) => oracle | ||
| .get_source_rate(¤cy, &source_name) | ||
| .await | ||
| .map_err(|e| anyhow!("Error getting rate from source: {e}"))?, | ||
| None => oracle | ||
| .get_median_rate(¤cy) | ||
| .await | ||
| .map_err(|e| anyhow!("Error getting median rate: {e}"))?, | ||
| }; | ||
|
|
||
| Ok(json!({ "rate": round_to_3dp(rate) })) | ||
| } | ||
|
|
||
| async fn listcurrencyrates(plugin: Plugin<PluginState>, args: Value) -> Result<Value, anyhow::Error> { | ||
| let currency = match args { | ||
| Value::Array(values) => { | ||
| if values.len() > 1 { | ||
| return Err(anyhow!( | ||
| "Too many arguments: expected at most 1 (currency), got {}", | ||
|
|
||
| values.len() | ||
| )); | ||
| } | ||
| let currency = values | ||
| .first() | ||
| .ok_or_else(|| anyhow!("Missing currency"))? | ||
|
|
@@ -212,7 +242,7 @@ async fn listcurrencyrates(plugin: Plugin<PluginState>, args: Value) -> Result<V | |
| .map(|source_result| { | ||
| json!({ | ||
| "source": source_result.name, | ||
| "amount": round_to_3dp(source_result.price), | ||
| }) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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 PR is scheduled for v26.06 so any fields added after currencyrate's v26.04 must have an
addedannotation. I went ahead and did that. Whenever schema files change we need to commit the generated files fromuv run make genas well. I also did that in another commit.