-
Notifications
You must be signed in to change notification settings - Fork 4
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/madara bootstrapper v0.0.2 #14
Conversation
Mohiiit
commented
Oct 23, 2024
- starknet rs version bump
- for declaring legacy, using the same types created in the madara
- zaun updated with latest core contract
…ionality uncommented
Tests Refactoring
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.
Nice! Few coms - first time checking this codebase so not sure what's relevant, maybe we don't want to move things too much. Feel free to self resolve comments if they don't make sense. :)
@@ -5,16 +5,26 @@ mod setup_scripts; | |||
pub mod tests; | |||
pub mod utils; | |||
|
|||
use clap::{ArgAction, Parser}; | |||
use std::fs::File; |
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.
More of a general comment but I think the code would gain clarity if the functions/structs only accepts clearly the arguments they need, instead of passing args
and config
everywhere, it makes it harder to understand what composes each structs/what functions uses without diving in the implementation
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.
I will add this in the refactor PR that we are planning
Argent, | ||
Braavos, | ||
} | ||
|
||
#[derive(Parser, Debug)] | ||
#[command(version, about, long_about = None)] | ||
pub struct CliArgs { |
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.
I guess a config file would make sense here, this would look hard to manage/update only through CLI (similar to the ChainConfig in madara)
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
@@ -5,16 +5,26 @@ mod setup_scripts; | |||
pub mod tests; | |||
pub mod utils; | |||
|
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.
We should use anyhow/eyre and have proper error handling
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.
this would be part of another PR, @cchudant already created one, I will rebase that after this PR
} else { | ||
starknet_accounts::ExecutionEncoding::New | ||
}; | ||
let signer = LocalWallet::from(SigningKey::from_secret_scalar(Felt::from_hex(private_key).unwrap())); |
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.
(nit) Cloning would help use removing this lifetime everywhere, right? Should probably be done at some point, the bootstraper doesn't really care about this small perf gain and the code would be simpler (in other places too)
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.
yes that's true, adding it to the refactor issue
@@ -17,7 +16,7 @@ use crate::contract_clients::core_contract::{ | |||
use crate::utils::convert_felt_to_u256; | |||
|
|||
pub struct StarknetValidityContract { |
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.
I don't have much knowledge about the bootstraper but why isn't it imported from Zaun?
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.
@apoorvsadana do you have context of this?
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.
we use it
pub struct StarknetValidityContract {
pub core_contract_client: StarknetValidityContractClient,
}
StarknetValidityContractClient
is from zaun
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.
we created a wrapper for our use case here
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.
ah yup, my bad
@@ -1,19 +1,15 @@ | |||
use std::path::Path; | |||
use std::time::Duration; | |||
use std::{fs, io}; | |||
|
|||
use ethers::addressbook::Address; | |||
use ethers::types::U256; |
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.
We should update to alloy
at some point since ethers is in the process of being deprecated
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.
yes we need to do that but we need to update zaun first, hence another PR for that refactor
src/utils/mod.rs
Outdated
pub fn convert_felt_to_u256(felt: StarkFelt) -> U256 { | ||
U256::from_big_endian(felt.bytes()) | ||
pub fn convert_felt_to_u256(felt: Felt) -> U256 { | ||
U256::from_big_endian(&felt.to_bytes_le()) |
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 we use from_big_endian
this should be felt.to_bytes_be()
, no? Else we can use U256::from_little_endian
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
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
src/tests/mod.rs
Outdated
@@ -106,11 +164,16 @@ fn get_config() -> CliArgs { | |||
config_hash_version: String::from(SN_OS_CONFIG_HASH_VERSION), | |||
app_chain_id: String::from(APP_CHAIN_ID), | |||
fee_token_address: String::from(FEE_TOKEN_ADDRESS), | |||
cross_chain_wait_time: 120, | |||
native_fee_token_address: String::from(NATIVE_FEE_TOKEN_ADDRESS), |
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.
So this is the default config for CliArgs
? We should probably just implement Default
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.
created
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.
Nice!