-
Notifications
You must be signed in to change notification settings - Fork 239
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: Add trace_get #987
feat: Add trace_get #987
Conversation
a213efe
to
a186031
Compare
Open to discussion about whether to use |
crates/provider/src/ext/trace.rs
Outdated
index: u64, | ||
) -> TransportResult<LocalizedTransactionTrace> { | ||
// We are using vec![indices] because API accepts a list, but in fact works only if list.len == 1 | ||
self.client().request("trace_get", (hash, vec![index])).await |
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.
you can use tuples for lists in JSON since they're equivalent to avoid allocating on the heap, see above in trace_transaction
crates/provider/src/ext/trace.rs
Outdated
index: u64, | ||
) -> TransportResult<LocalizedTransactionTrace> { | ||
// We are using vec![indices] because API accepts a list, but in fact works only if list.len == 1 | ||
self.client().request("trace_get", (hash, vec![index])).await |
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 should also use the Index
type of rpc-types-eth, because this should ideally be formatted as hex
crates/provider/src/ext/trace.rs
Outdated
async fn trace_get( | ||
&self, | ||
hash: TxHash, | ||
index: 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.
since this is an index, I wonder if we should use usize here because I assume most of the time this value is derived from doing some vec.len() +- ops
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.
That's a good point, put usize in there.
I think it would be less convenient to put Index as type because then all calls to trace_get would require a .into() call from the user. It probably would be less convenient.
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.
lgtm,
doc nit
docs Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@mattsse could you approve workflows, please? |
* trace_get implementation * Review * Update crates/provider/src/ext/trace.rs docs Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de> * Cargo fmt --------- Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Motivation
Adds the missing trace_get method in #926
Problems
Currently trace_get returns null if queries with multiple indices (params example: "params":["0x346b152d421c095a2eab383d9c5d0309bb28c31e8c5b84fdc6a4f38e2194558d", ["0x0", "0x1"]]) paradigmxyz/reth#3852
Because of this, i choose to use param
index
with type u64, to remove footgun.Solution
Usage example. Replace RETH_ENDPOINT with endpoint
PR Checklist