-
Notifications
You must be signed in to change notification settings - Fork 5
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
2023 12 23 ob crate #51
Conversation
orderbook-rs/src/gasoracle/mod.rs
Outdated
let client = Client::new(); | ||
let url = format!( | ||
"{}{}", | ||
"https://api.blocknative.com/gasprices/blockprices?chainid=", chain_id |
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.
magic strings like this should probably be a constant, especially if we plan to ever hit more than one endpoint for an API
also there is a url crate fyi https://docs.rs/url/latest/url/
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.
Resolved.
orderbook-rs/src/gasoracle/mod.rs
Outdated
request = request.header(AUTHORIZATION, api_key); | ||
} | ||
let response: BlockNativeResponse = request.send().await?.error_for_status()?.json().await?; | ||
let fatest = response |
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.
typo
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.
Resolved
orderbook-rs/src/interpreter/mod.rs
Outdated
let provider = match Provider::<Http>::try_from(rpc_url.clone()) { | ||
Ok(provider) => provider, | ||
Err(err) => { | ||
error!("INVALID RPC URL"); |
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.
probably if this is to be a crate we want an error enum rather than strings, generally anyhow is probably better for binaries than libs
this is because if we expect someone else to use this crate as a dependency, they may want to change the behaviour of their code based on what happens in this code, in which case an enum allows them to do something like match error { RainOrderbookError::InvalidRPC => { ... } }
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.
Added error enum.
orderbook-rs/src/interpreter/mod.rs
Outdated
let provider = match Provider::<Http>::try_from(rpc_url.clone()) { | ||
Ok(provider) => provider, | ||
Err(err) => { | ||
error!("INVALID RPC URL"); |
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.
if you're using the same error in several locations as a return, it's more weight to the idea that you want an enum
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.
Resolved.
abigen!(IParserV1, "../out/IParserV1.sol/IParserV1.json"); | ||
abigen!( | ||
IExpressionDeployerV3, | ||
r#"[ |
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.
magic strings should be constants if we can
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.
Macros do not accept &str constants as parameters.
No description provided.