-
Notifications
You must be signed in to change notification settings - Fork 11
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
adds kind
to RunTx, and SpamTx Plan
#30
Conversation
2548563
to
44d23ec
Compare
477c9b3
to
3078381
Compare
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.
looks good! Just had a few comments
assert_eq!(spam[0].fuzz.as_ref().unwrap()[0].param, "amountIn"); | ||
assert_eq!( |
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 assertion (or something similar) should still be in 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.
I modified the config toml so that we are unit testing a small, easily understandable file. check the testConfig.toml I wrote.
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.
yeah I just mean we should continue to test the fuzz params using that file. This test was just deleted but it doesn't have to be.
println!("contract address: {:?}", receipt.contract_address | ||
.as_ref() | ||
.map(|a| a.encode_hex()) | ||
.unwrap_or("".to_string())); |
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.
does unwrap_or_default
work 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.
says default trait is not implemented
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.
Default is implemented for String, might just be a syntax issue
pub fn with_name(name: &str, tx: TransactionRequest) -> Self { | ||
Self { | ||
name: Some(name.to_string()), | ||
kind: None, | ||
tx, | ||
} | ||
} | ||
|
||
pub fn set_kind(&mut self, kind: String) { | ||
self.kind = Some(kind); | ||
} |
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 think this all should just be swapped out for a new
constructor.
impl NamedTxRequest {
pub fn new(tx: TransactionRequest, name: Option<&str>, kind: Option<&str>) -> Self {...}
}
This is more idiomatic, and we don't have any reason to iteratively build the struct in our code in the first place.
} | ||
|
||
impl PendingRunTx { | ||
pub fn new(tx_hash: TxHash, start_timestamp: usize) -> Self { | ||
pub fn new(tx_hash: TxHash, start_timestamp: usize, kind: Option<String>) -> Self { |
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'd change this to kind: Option<&str>
-- that way the user can call it with Some("kind")
No description provided.