-
Notifications
You must be signed in to change notification settings - Fork 52
chore: relayer shared utils #684
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #684 +/- ##
=======================================
Coverage 99.86% 99.86%
=======================================
Files 17 17
Lines 767 767
=======================================
Hits 766 766
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 for this. I added my comments
| /// Returns a `Status` error if any transaction ID is not exactly 32 bytes | ||
| #[inline] | ||
| #[allow(clippy::result_large_err)] | ||
| pub fn parse_eth_tx_hashes(tx_ids: Vec<Vec<u8>>) -> Result<Vec<[u8; 32]>, Status> { |
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.
Isn't it better to return TxHash?
| .into_iter() | ||
| .map(Hash::try_from) | ||
| .collect::<Result<Vec<_>, _>>() | ||
| .map_err(|e| Status::from_error(e.into())) |
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.
probably should use to_tonic_status from above.
| .map_err(|tx| format!("invalid tx hash: {tx:?}")) | ||
| }) | ||
| .collect::<Result<Vec<[u8; 32]>, _>>() | ||
| .map_err(|e| Status::from_error(e.into())) |
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.
ditto: probably should use to_tonic_status
| /// | ||
| /// Returns a `ClientState` with default trust level, max clock drift, and the provided proof specs | ||
| #[must_use] | ||
| pub fn build_tendermint_client_state( |
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.
Maybe this function should also take trust_level: Option<Fraction> and max_clock_drift: Option<Duration>.
And if these fields are None, then we use the default values?
| unused_crate_dependencies | ||
| )] | ||
|
|
||
| use tendermint as _; |
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.
what does this do?
| use ics23 as _; | ||
| use tendermint as _; | ||
|
|
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.
are these needed?
Description
Reduced code duplication
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.