Conversation
6f0ec87 to
cccac84
Compare
|
@greptile review the PR according to the CONTRIBUTION.md |
Greptile OverviewGreptile SummaryThis PR adds BTC/USD price fetching functionality from CoinGecko API, displaying the current Bitcoin price in the positions view. Key changes:
Addressed from previous feedback:
The implementation is clean, follows Rust best practices, and properly handles the blocking I/O in an async context. Confidence Score: 4.5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant Positions
participant PriceFetcher
participant CoinGecko
User->>CLI: cargo run positions
CLI->>Positions: run_positions()
Positions->>PriceFetcher: spawn_blocking(fetch_btc_usd_price)
PriceFetcher->>CoinGecko: GET /api/v3/simple/price
alt Success (200)
CoinGecko-->>PriceFetcher: {"bitcoin":{"usd":price}}
PriceFetcher-->>Positions: Ok(price)
Positions->>User: Display "$price"
else Rate Limited (429)
CoinGecko-->>PriceFetcher: 429 status
PriceFetcher-->>Positions: Err(RateLimit)
Positions->>User: Display "Rate limit exceeded"
else Other Error
CoinGecko-->>PriceFetcher: Error/timeout
PriceFetcher-->>Positions: Err(error)
Positions->>User: Display "Price fetcher service unavailable"
end
Positions->>User: Display positions with price info
|
Greptile's behavior is changing!From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
cccac84 to
7999cfe
Compare
|
@greptile review the PR according to the CONTRIBUTION.md |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@greptile review the PR according to the CONTRIBUTION.md |
7999cfe to
4a7a31a
Compare
|
@greptile review the PR according to the CONTRIBUTION.md |
crates/cli-client/Cargo.toml
Outdated
| nostr-sdk = { version = "0.44.1" } | ||
|
|
||
| minreq = { version = "2.14", features = ["https", "json-using-serde"] } | ||
| reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] } |
|
|
||
| #[derive(Deserialize)] | ||
| struct BitcoinPrice { | ||
| usd: Decimal, |
There was a problem hiding this comment.
Why do we need rust_decimal::Decimal?
crates/cli-client/src/cli/price.rs
Outdated
| use rust_decimal::Decimal; | ||
|
|
||
| pub async fn fetch_btc_usd_price<T: PriceFetcher>(fetcher: &T) -> Result<Decimal, Error> { | ||
| Ok(fetcher.fetch_price().await?) |
There was a problem hiding this comment.
| Ok(fetcher.fetch_price().await?) | |
| fetcher.fetch_price().await |
crates/cli-client/src/cli/price.rs
Outdated
| pub async fn run_price_feed() -> Result<(), Error> { | ||
| let fetcher = CoingeckoPriceFetcher::new()?; | ||
| let price = fetch_btc_usd_price(&fetcher).await?; | ||
|
|
||
| println!("1 BTC = ${}", price.round_dp(2)); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
What is the purpose of this function?
There was a problem hiding this comment.
The purpose is to show the price in the terminal using the command cargo run -- price-feed (according to what was written in the original issue: #40). Don’t we need it?
crates/cli-client/src/cli/price.rs
Outdated
| pub async fn fetch_btc_usd_price<T: PriceFetcher>(fetcher: &T) -> Result<Decimal, Error> { | ||
| Ok(fetcher.fetch_price().await?) | ||
| } | ||
|
|
There was a problem hiding this comment.
Let's move this to the: crates/cli-client/src/price_fetcher.rs
7ff0601 to
a0b1d27
Compare
|
@greptile review the PR according to the CONTRIBUTION.md |
| let fetcher = CoingeckoPriceFetcher::new(); | ||
| let btc_price = fetch_btc_usd_price(&fetcher) | ||
| .map(|x| format!("{x:.2}$")) | ||
| .unwrap_or("'Rate limit exceed'".to_string()); |
There was a problem hiding this comment.
[P1] Swallows non-rate-limit errors as "rate limit"
fetch_btc_usd_price(&fetcher) can fail for reasons other than a 429 (e.g., no network, DNS issues, CoinGecko 5xx, JSON shape changes), but this unwrap_or("'Rate limit exceed'") prints the same message for all failures, which makes debugging and user guidance misleading. Consider matching on the error and only mapping PriceFetcherError::RateLimit to that message, while showing (or logging) other errors.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/cli-client/src/cli/positions.rs
Line: 31:34
Comment:
[P1] Swallows non-rate-limit errors as "rate limit"
`fetch_btc_usd_price(&fetcher)` can fail for reasons other than a 429 (e.g., no network, DNS issues, CoinGecko 5xx, JSON shape changes), but this `unwrap_or("'Rate limit exceed'")` prints the same message for all failures, which makes debugging and user guidance misleading. Consider matching on the error and only mapping `PriceFetcherError::RateLimit` to that message, while showing (or logging) other errors.
How can I resolve this? If you propose a fix, please make it concise.| let fetcher = CoingeckoPriceFetcher::new(); | ||
| let btc_price = fetch_btc_usd_price(&fetcher) | ||
| .map(|x| format!("{x:.2}$")) | ||
| .unwrap_or("'Rate limit exceed'".to_string()); |
There was a problem hiding this comment.
[P3] User-facing string has typo/quoting
"'Rate limit exceed'" reads like a debug string (extra quotes) and has a grammar typo; it will show up in normal CLI output. Consider a cleaner message like "Rate limit exceeded".
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/cli-client/src/cli/positions.rs
Line: 34:34
Comment:
[P3] User-facing string has typo/quoting
`"'Rate limit exceed'"` reads like a debug string (extra quotes) and has a grammar typo; it will show up in normal CLI output. Consider a cleaner message like `"Rate limit exceeded"`.
How can I resolve this? If you propose a fix, please make it concise.| pub trait PriceFetcher { | ||
| fn fetch_price(&self) -> Result<f64, Error>; | ||
| } |
There was a problem hiding this comment.
[P1] Trait returns crate-wide Error instead of PriceFetcherError
PriceFetcher::fetch_price() returning Result<f64, Error> makes the trait harder to reuse and mock because it couples the fetcher layer to the entire CLI error enum. Since this module already defines PriceFetcherError, consider making the trait return Result<f64, PriceFetcherError> and converting to crate::error::Error at the CLI boundary.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/cli-client/src/price_fetcher.rs
Line: 27:29
Comment:
[P1] Trait returns crate-wide `Error` instead of `PriceFetcherError`
`PriceFetcher::fetch_price()` returning `Result<f64, Error>` makes the trait harder to reuse and mock because it couples the fetcher layer to the entire CLI error enum. Since this module already defines `PriceFetcherError`, consider making the trait return `Result<f64, PriceFetcherError>` and converting to `crate::error::Error` at the CLI boundary.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.a0b1d27 to
e2ed30c
Compare
|
@greptile review the PR according to the CONTRIBUTION.md |
| pub trait PriceFetcher { | ||
| fn fetch_price(&self) -> Result<f64, PriceFetcherError>; |
There was a problem hiding this comment.
Trait uses synchronous blocking HTTP in async context. fetch_price() calls blocking minreq::get() which blocks the tokio runtime thread. Either mark trait method as async or use tokio::task::spawn_blocking() when calling it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/cli-client/src/price_fetcher.rs
Line: 26:27
Comment:
Trait uses synchronous blocking HTTP in async context. `fetch_price()` calls blocking `minreq::get()` which blocks the tokio runtime thread. Either mark trait method as async or use `tokio::task::spawn_blocking()` when calling it.
How can I resolve this? If you propose a fix, please make it concise.| pub fn new() -> Self { | ||
| Self {} | ||
| } |
There was a problem hiding this comment.
Missing Default trait implementation. new() creates empty struct - derive or implement Default instead.
| pub fn new() -> Self { | |
| Self {} | |
| } | |
| #[must_use] | |
| pub const fn new() -> Self { | |
| Self | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/cli-client/src/price_fetcher.rs
Line: 36:38
Comment:
Missing `Default` trait implementation. `new()` creates empty struct - derive or implement `Default` instead.
```suggestion
#[must_use]
pub const fn new() -> Self {
Self
}
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| let fetcher = CoingeckoPriceFetcher::new(); | ||
| let btc_price = match fetch_btc_usd_price(&fetcher) { | ||
| Ok(price) => format!("{price:.2}$"), | ||
| Err(PriceFetcherError::RateLimit) => "Rate limit exceeded".to_string(), | ||
| Err(e) => { | ||
| eprintln!("Debug error: {e}"); | ||
| "Price fetcher service unavailable".to_string() | ||
| } | ||
| }; |
There was a problem hiding this comment.
Blocking HTTP call in async function. fetch_btc_usd_price() uses synchronous minreq and will block the tokio runtime thread. Wrap in tokio::task::spawn_blocking():
let btc_price = match tokio::task::spawn_blocking(move || fetch_btc_usd_price(&fetcher)).await {
Ok(Ok(price)) => format!("{price:.2}$"),
Ok(Err(PriceFetcherError::RateLimit)) => "Rate limit exceeded".to_string(),
Ok(Err(e)) => {
eprintln!("Debug error: {e}");
"Price fetcher service unavailable".to_string()
}
Err(_) => "Price fetcher service unavailable".to_string(),
};
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/cli-client/src/cli/positions.rs
Line: 31:39
Comment:
Blocking HTTP call in async function. `fetch_btc_usd_price()` uses synchronous `minreq` and will block the tokio runtime thread. Wrap in `tokio::task::spawn_blocking()`:
```
let btc_price = match tokio::task::spawn_blocking(move || fetch_btc_usd_price(&fetcher)).await {
Ok(Ok(price)) => format!("{price:.2}$"),
Ok(Err(PriceFetcherError::RateLimit)) => "Rate limit exceeded".to_string(),
Ok(Err(e)) => {
eprintln!("Debug error: {e}");
"Price fetcher service unavailable".to_string()
}
Err(_) => "Price fetcher service unavailable".to_string(),
};
```
How can I resolve this? If you propose a fix, please make it concise.e2ed30c to
09f84cd
Compare
|
@greptile review the PR according to the CONTRIBUTION.md |
| pub struct CoingeckoPriceFetcher; | ||
|
|
||
| impl CoingeckoPriceFetcher { | ||
| const URL: &'static str = "https://api.coingecko.com/api/v3/simple/price?ids=bitcoin&vs_currencies=usd&precision=8"; | ||
| const TIMEOUT_SECS: u64 = 5; | ||
|
|
||
| #[must_use] | ||
| pub fn new() -> Self { | ||
| Self | ||
| } |
There was a problem hiding this comment.
derive Default instead of implementing new() for zero-field struct
| pub struct CoingeckoPriceFetcher; | |
| impl CoingeckoPriceFetcher { | |
| const URL: &'static str = "https://api.coingecko.com/api/v3/simple/price?ids=bitcoin&vs_currencies=usd&precision=8"; | |
| const TIMEOUT_SECS: u64 = 5; | |
| #[must_use] | |
| pub fn new() -> Self { | |
| Self | |
| } | |
| #[derive(Default)] | |
| pub struct CoingeckoPriceFetcher; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/cli-client/src/price_fetcher.rs
Line: 32:41
Comment:
derive `Default` instead of implementing `new()` for zero-field struct
```suggestion
#[derive(Default)]
pub struct CoingeckoPriceFetcher;
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| .unwrap_or_else(|e| Err(PriceFetcherError::Internal(e.to_string()))); | ||
|
|
||
| let btc_price = match btc_result { | ||
| Ok(price) => format!("{price:.2}$"), |
There was a problem hiding this comment.
inconsistent currency symbol placement - should be ${price:.2} (USD convention)
| Ok(price) => format!("{price:.2}$"), | |
| Ok(price) => format!("${price:.2}"), |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/cli-client/src/cli/positions.rs
Line: 37:37
Comment:
inconsistent currency symbol placement - should be `${price:.2}` (USD convention)
```suggestion
Ok(price) => format!("${price:.2}"),
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.09f84cd to
99e8903
Compare
|
@greptile review the PR according to the CONTRIBUTION.md |
|
Rebase onto main pls, also bump to the last simplicityhl-core version and fix compilation errors |
99e8903 to
943b97f
Compare
| let price = match resp.status_code { | ||
| 200 => resp | ||
| .json::<BitcoinResponse>() | ||
| .map(|data| data.bitcoin.usd) | ||
| .map_err(|e| PriceFetcherError::Parse(e.to_string()))?, | ||
| 429 => return Err(PriceFetcherError::RateLimit), | ||
| status => return Err(PriceFetcherError::Status(status)), | ||
| }; | ||
|
|
||
| Ok(price) |
There was a problem hiding this comment.
| let price = match resp.status_code { | |
| 200 => resp | |
| .json::<BitcoinResponse>() | |
| .map(|data| data.bitcoin.usd) | |
| .map_err(|e| PriceFetcherError::Parse(e.to_string()))?, | |
| 429 => return Err(PriceFetcherError::RateLimit), | |
| status => return Err(PriceFetcherError::Status(status)), | |
| }; | |
| Ok(price) | |
| match resp.status_code { | |
| 200 => resp | |
| .json::<BitcoinResponse>() | |
| .map(|data| data.bitcoin.usd) | |
| .map_err(|e| PriceFetcherError::Parse(e.to_string()))?, | |
| 429 => Err(PriceFetcherError::RateLimit), | |
| status => Err(PriceFetcherError::Status(status)), | |
| } |
chore: * bump simplicityhl version from 0.4.0 -> 0.4.1 * remove Cargo.lock from .gitignore
943b97f to
236885d
Compare
Usage: cargo run -p cli-client price-feed
Fix: #40