Suggest improvements #1
Conversation
- chain struct that allows specification of custom optional rpc url
- setup logging - setup sled
This commit adds: - Chainlink aggregator ABI - Async price updater - Contract wrapper for chainlink contracts - Tests
src/lib.rs
Outdated
|
|
||
| // Try to initialize and catch error silently if already initialized | ||
| // during tests this make this function throw error | ||
| if log4rs::init_config(config).is_err() { |
There was a problem hiding this comment.
Not sure if it is a good idea to initialize a logger from within a library... like tests should perhaps create their own logger? Like I would hate if this somehow interferes with my env when I load this lib.
There was a problem hiding this comment.
Actually, this like totally freaks me out. + .gitignore won't help here as logs will be created inside the folder of an application using this library. I suggest either removing this completely or making logging optional via some configuration flag. A local database or configuration files is understandable, but a library/crate creating its own logs without any external control over it is a rather bad etiquette.
I would perhaps suggest accepting Option<&Path> that contains a log file name or if None, logging is skipped across the entire library.
There was a problem hiding this comment.
Good point, I had a custom logging mechanism before but was told that this is bad practice and that RUST_LOG env should be used.
src/interface/mod.rs
Outdated
| pub price: f64, | ||
| } | ||
|
|
||
| impl Serializable for PriceData { |
There was a problem hiding this comment.
Serializable trait and impl look useless here as you can invoke bincode::serialize() directly on PriceData.
src/interface/mod.rs
Outdated
|
|
||
| /// The latest price received for this symbol. | ||
| /// This data is directly retrieved from the underlying contract. | ||
| #[derive(Deserialize, Serialize)] |
There was a problem hiding this comment.
Consider adding Debug to derive so that you can use println!("{:#?}", price_data); in tests to get formatted output.
src/fetcher/mod.rs
Outdated
| let address = &contract_configuration.1; | ||
| match fetch_price_for_contract(crypto_prices.clone().configuration, address).await { | ||
| Ok(price_data) => { | ||
| match crypto_prices |
There was a problem hiding this comment.
If this behavior is intended, then it is cleaner to use:
if let Err(err) = crypto_prices.tree.insert( ... ) {
log::error!("...");
}
src/fetcher/mod.rs
Outdated
|
|
||
| /// Retrieves the price of an underlying asset from a particular contract | ||
| async fn fetch_price_for_contract( | ||
| rustlink_configuration: Configuration, |
There was a problem hiding this comment.
Perhaps passing by reference here could be more efficient?
src/fetcher/mod.rs
Outdated
| for contract_configuration in contracts { | ||
| let symbol = &contract_configuration.0; | ||
| let address = &contract_configuration.1; | ||
| match fetch_price_for_contract(crypto_prices.clone().configuration, address).await { |
There was a problem hiding this comment.
If you pass configuration by reference, you might not need to clone? (AFAICS)
| @@ -0,0 +1,166 @@ | |||
| mod fetcher; | |||
There was a problem hiding this comment.
I personally don't really like to create implementations in lib.rs, but there are no written rules about this. Just if the project grows it can become a bit cluttered. I would consider creating config.rs and error.rs containing appropriate structs.
Also, I would say that it is more common for crates to export Error as opposed to DatabaseError as any importer can import use xxx::Error as DatabaseError.
src/fetcher/mod.rs
Outdated
| match crypto_prices | ||
| .tree | ||
| .insert(symbol, price_data.to_bin().unwrap()) | ||
| { |
There was a problem hiding this comment.
I think you could perhaps use more idiomatic rust here:
fetch_price_for_contract(crypto_prices.clone().configuration, address)
.await
.and_then(|data| crypto_prices.tree.insert(...))
.or_else(|err| err);... or along these lines ...
src/lib.rs
Outdated
| pub fn new( | ||
| rpc_url: &str, | ||
| fetch_interval_seconds: u64, | ||
| sled_path: &str, |
There was a problem hiding this comment.
Paths are typically handled via std::path::Path or a generic can also be useful here such as:
pub fn new<P : AsRef<Path>>( ..., sled_path: P, ...) {
let db = sled::open(sled_path.as_ref()).unwrap();
...
}
src/lib.rs
Outdated
| fetch_interval_seconds: u64, | ||
| sled_path: &str, | ||
| contracts: Vec<(String, String)>, | ||
| ) -> CryptoPrices { |
There was a problem hiding this comment.
Change CryptoPrices to Self
Perhaps it would make sense to change new() -> Self to try_new() -> Result<Self> and handle sled opening as let db = sled::open(sled_path)?; to cascade the error to the invocator?
Todo: deprecate logging, DB and introduce channels Co-Authored-By: aspect <7544020+aspect@users.noreply.github.com>
- add reflector enum that allows specification on how to receive round data Todo: deprecate global logging configuration in favor of custom logging functionality
- Added WASM compatibility and WASM build scripts - Added graceful shutdown - Deprecated global logging configuration
|
@aspect Relevant changes made, a review is appreciated. |
Add metadata
Update module name
No description provided.