-
Notifications
You must be signed in to change notification settings - Fork 2
Added fetch_add_nonce method for NodeInteraction trait. Added extra logging. #25
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
Conversation
xermicus
left a comment
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.
Thanks, looks good! Just some smallies
crates/core/src/driver/mod.rs
Outdated
| task.json_output = Some(output.output.clone()); | ||
| task.error = output.error; | ||
| self.contracts.insert(output.input, output.output); | ||
| self.contracts.insert(output.input, output.output.clone()); |
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.
Is the added .clone() necessary, I don't see the output being accessed further down?
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.
Indeed, it's not necessary.
| } | ||
| } | ||
| } else { | ||
| log::warn!("Compiled contracts field is None"); |
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 like the only point of this heavily nested code here is to log a warning if there were no contracts compiled? This makes sense but could be simplified a lot.
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.
Actually it is useful to log that the contract was succesfully compiled from a file and also when we don't have any contract to compile.
| .with_input(Bytes::from(code.clone())) | ||
| .with_gas_price(20_000_000_000) | ||
| .with_gas_limit(20_000_000_000) | ||
| .with_gas_price(5_000_000) |
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.
Why did you need to change this? IIRC it was arbitrary but if it has to be a specific value it would be good to have the reason as comment.
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 have changed it because I have received this message:
error code -32000: tx fee (400.00 ether) exceeds the configured cap (1.00 ether)
And this is because:
Total Fee = gas_price * gas_limit
= 20_000_000_000 * 20_000_000
= 400_000_000_000_000_000_000 (wei)
= 400 ether
And the new value will be:
0.000025 ether
| /// Returns the state diff of the transaction hash in the [TransactionReceipt]. | ||
| fn state_diff(&self, transaction: TransactionReceipt) -> anyhow::Result<DiffMode>; | ||
|
|
||
| fn fetch_add_nonce(&self, address: Address) -> anyhow::Result<u64>; |
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.
Please add a doc comment explaining what the trait method is supposed to do.
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.
Fixed in the new revision
Added fetch_add_nonce method for NodeInteraction trait.
Improved logging for some components.