Suggest improvements #1

starkbamse · 2024-05-13T19:50:21Z

No description provided.

- 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

aspect · 2024-05-13T20:05:43Z

src/lib.rs

+
+        // 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() {


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.

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.

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.

aspect · 2024-05-13T20:06:28Z

src/interface/mod.rs

+    pub price: f64,
+}
+
+impl Serializable for PriceData {


Serializable trait and impl look useless here as you can invoke bincode::serialize() directly on PriceData.

aspect · 2024-05-13T20:08:52Z

src/interface/mod.rs

+
+/// The latest price received for this symbol.
+/// This data is directly retrieved from the underlying contract.
+#[derive(Deserialize, Serialize)]


Consider adding Debug to derive so that you can use println!("{:#?}", price_data); in tests to get formatted output.

aspect · 2024-05-13T20:14:25Z

src/fetcher/mod.rs

+            let address = &contract_configuration.1;
+            match fetch_price_for_contract(crypto_prices.clone().configuration, address).await {
+                Ok(price_data) => {
+                    match crypto_prices


If this behavior is intended, then it is cleaner to use:

if let Err(err) = crypto_prices.tree.insert( ... ) { log::error!("..."); }

aspect · 2024-05-13T20:15:21Z

src/fetcher/mod.rs

+
+/// Retrieves the price of an underlying asset from a particular contract
+async fn fetch_price_for_contract(
+    rustlink_configuration: Configuration,


Perhaps passing by reference here could be more efficient?

aspect · 2024-05-13T20:15:52Z

src/fetcher/mod.rs

+        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 {


If you pass configuration by reference, you might not need to clone? (AFAICS)

aspect · 2024-05-13T20:24:21Z

src/lib.rs

@@ -0,0 +1,166 @@
+mod fetcher;


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.

aspect · 2024-05-13T20:24:26Z

src/fetcher/mod.rs

+                    match crypto_prices
+                        .tree
+                        .insert(symbol, price_data.to_bin().unwrap())
+                    {


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 ...

aspect · 2024-05-13T20:39:40Z

src/lib.rs

+    pub fn new(
+        rpc_url: &str,
+        fetch_interval_seconds: u64,
+        sled_path: &str,


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(); ... }

aspect · 2024-05-13T20:41:32Z

src/lib.rs

+        fetch_interval_seconds: u64,
+        sled_path: &str,
+        contracts: Vec<(String, String)>,
+    ) -> CryptoPrices {


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 <[email protected]>

- 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

starkbamse · 2024-05-15T15:45:56Z

@aspect Relevant changes made, a review is appreciated.

Add metadata

Update module name

starkbamse added 4 commits May 10, 2024 22:29

Create chain struct, placeholder contracts

d79ff9e

- chain struct that allows specification of custom optional rpc url

Finalized rustlink struct

5b51534

- setup logging - setup sled

ABI, fetcher, contract abstraction

65bb751

This commit adds: - Chainlink aggregator ABI - Async price updater - Contract wrapper for chainlink contracts - Tests

Update README.md

9591430

aspect reviewed May 13, 2024

View reviewed changes

starkbamse and others added 5 commits May 14, 2024 01:53

Resolve PR suggestions, rename

ffb94e6

Todo: deprecate logging, DB and introduce channels Co-Authored-By: aspect <[email protected]>

Update README.md

fa83944

Deprecate databases introduce channels

d1caf93

- add reflector enum that allows specification on how to receive round data Todo: deprecate global logging configuration in favor of custom logging functionality

WASM, shutdown signals, logging, js example

dae3fee

- Added WASM compatibility and WASM build scripts - Added graceful shutdown - Deprecated global logging configuration

Format

cb75285

starkbamse added 9 commits May 16, 2024 02:41

Formatting

8acc102

Create rust.yml

d7911a8

Update readme

8016a4b

Merge branch 'main' of https://github.com/starkbamse/rustlink

790a88b

Add metadata and WASM usage in README

dcebaf0

Delete js folder

2039563

Update .gitignore

76a00e5

Merge pull request #2 from starkbamse/add-metadata

c69d2f9

Add metadata

Update module name

1719505

starkbamse closed this May 16, 2024

Merge pull request #3 from starkbamse/update-module-name

fbba781

Update module name

starkbamse reopened this May 16, 2024

starkbamse merged commit 14a2115 into suggest-changes May 16, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest improvements #1

Suggest improvements #1

starkbamse commented May 13, 2024

aspect May 13, 2024

aspect May 13, 2024

starkbamse May 13, 2024

aspect May 13, 2024

aspect May 13, 2024

aspect May 13, 2024

aspect May 13, 2024

aspect May 13, 2024

aspect May 13, 2024

aspect May 13, 2024

aspect May 13, 2024

aspect May 13, 2024

starkbamse commented May 15, 2024

Suggest improvements #1

Suggest improvements #1

Conversation

starkbamse commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starkbamse commented May 15, 2024