Skip to content
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

Feat: random bundles test #23

Merged
merged 14 commits into from
May 9, 2023
Merged

Feat: random bundles test #23

merged 14 commits into from
May 9, 2023

Conversation

bluekirby1111
Copy link
Contributor

Adds random bundles generation and tests within rust-sdk

@bluekirby1111 bluekirby1111 changed the title Feat/random bundles test Feat: random bundles test Mar 7, 2023
Copy link
Contributor

@mikonieminen mikonieminen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the changes can be done with separate refactoring, but those should be tracked as separate tickets. If moved to separate ticket, reply in the comment with ticket ID.

examples/fund.rs Outdated Show resolved Hide resolved
examples/fund.rs Outdated Show resolved Hide resolved
examples/fund.rs Outdated
@@ -7,8 +7,10 @@ use reqwest::Url;
async fn main() {
let url = Url::parse("https://node1.bundlr.network").unwrap();
let wallet = PathBuf::from_str("res/test_wallet.json").unwrap();
let currency = Arweave::new(wallet, None);
let bundlr = Bundlr::new(url, &currency).await;
let currency = Arweave::new(wallet, None).expect("Could not create currency");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this design is really weird. Why would you pass wallet or any other arguments to the currency instance? There's something missing with this abstraction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some functionalities from Currency needs private key. The main idea was to have this as an option

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When looking at the Currency trait, I can see there are some mistakes in the design (which require you to pass wallet/signer to the Currency instance):

  1. get_tx and get_tx_status are really weird ones there, why would you ask information about transactions from a currency?
  2. get_current_height, what does this to do with a currency?
  3. all signing and verifying of transactions, these are signer's (or wallet's) functionalities, not currency's
  4. price, this is either not something a currency should provide, this is something exchange tells you

Looking at the trait, about 90% of it is in a wrong place.

examples/upload.rs Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/index.rs Show resolved Hide resolved

#[cfg(feature = "aptos")]
use crate::MultiAptosSigner;

use crate::error::BundlrError;

#[derive(FromPrimitive, Display, PartialEq, Eq, Debug, Clone)]
pub enum SignerMap {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to bring this up quite some time so doing it now: this type name is quite bad, it always makes me wonder what is this about until I look at the definition. Maybe this should be renamed to SignerType or something else. In the end, it's not a Map type, it's an enumeration.

Comment on lines +58 to +59
<[u8; 2]>::try_from(sig_type_b)
.map_err(|err| BundlrError::BytesError(err.to_string()))?,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this to a separate variable and pass it here. Would make the code a bit cleaner.

tsconfig.json Outdated
"strict": true,
"resolveJsonModule": true
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing new line at the end of the file. Please, fix your editor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move all of this nodejs specific code into a separate subdir.

node_modules
dist
/res/gen_bundles/bundle*
bundles.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline. Fix your editor.

@bluekirby1111
Copy link
Contributor Author

Smaller issues noted, I'll address them soon

@bluekirby1111 bluekirby1111 merged commit c7da15d into master May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants