-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: resolve multiple function/event selectors in one openchain.xyz request #6719
Conversation
} | ||
|
||
/// Identifies `Function` from its cache or `https://api.openchain.xyz` | ||
pub async fn identify_function(&mut self, identifier: &[u8]) -> Option<Function> { | ||
self.ensure_not_offline()?; |
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 want to use results from loaded disk cache, network requests will be ignored in identify()
function
} | ||
|
||
#[derive(Deserialize)] | ||
struct ApiResult { | ||
event: HashMap<String, Vec<Decoded>>, | ||
function: HashMap<String, Vec<Decoded>>, | ||
event: HashMap<String, Option<Vec<Decoded>>>, |
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.
api returns null
for unknown selectors, we don't want to fail whole request with probably success responses
#[derive(Deserialize)] | ||
struct Decoded { | ||
name: String, | ||
filtered: bool, |
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.
filter=true
by default (source), so results are already filtered on API side and this field is not in response.
hey @cdump — def no more big changes planned for this section of code, sorry about that! feel free to go ahead and rebase |
Rebased & ready for review. |
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, sorry for the wait
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, needs clippy
@Evalir, what do you mean by 'needs clippy'? It seems that other MR no longer fail on clippy nightly. Unfortunately, I don't have the rights to restart the clippy check in this MR, should I force-push just to restart pipeline? |
Motivation
Currently, the number of HTTP requests to openchain.xyz is equal to the number of selectors to resolve, which is not optimal.
On the current
master
, the followingcast run
takes ~11 seconds:I've added
println!("get {url}");
to selectors.rs:58 and got:Solution
openchain.xyz's api supports multiple selectors in one request - let's use that feature.
After the proposed changes,
cast run ...
took 2.8 seconds - x4 faster than before