Skip to content
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

Send solver competition info to api from driver #193

Merged
merged 3 commits into from
May 5, 2022
Merged

Conversation

vkgnosis
Copy link
Contributor

@vkgnosis vkgnosis commented May 5, 2022

Part of #127 .

There are some minor todos that I don't want to be blocking the main part of this PR so I want to merge this part first.

Test Plan

CI and manual verification that this works

@vkgnosis vkgnosis requested a review from a team as a code owner May 5, 2022 13:02
@vkgnosis vkgnosis changed the title Send solver competitoin info to api from driver Send solver competition info to api from driver May 5, 2022
Comment on lines +618 to +619
// TODO: we don't have access to this and there is no guarantee there is one such block
competition_simulation_block: 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding this TODO and the one below - do you think it makes sense to return this along with the simulations that we already do?

Specifically we can add a eth_getBlockNumber(latest) to the request batch and return the computed calldata along with the simulation result that computes the gas used.

You mentioned below that some refactoring to make this happen, I'm not sure if this is what you had in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For both the todos we need a little bit of refactoring yeah. The reason here it doesn't completely work is that we split up over multiple batches if there are too many simulations. These batches could happen in different block numbers. (Really we don't even have a guarantee that a batch always happes in the same block either.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really we don't even have a guarantee that a batch always happes in the same block either.

Oh yeah, that's a good point. It may be nice as an indicative value anyway even if its not accurate.

Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@codecov-commenter
Copy link

codecov-commenter commented May 5, 2022

Codecov Report

Merging #193 (0e6461f) into main (8a412ec) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #193      +/-   ##
==========================================
- Coverage   64.86%   64.74%   -0.12%     
==========================================
  Files         190      190              
  Lines       38966    39033      +67     
==========================================
- Hits        25275    25273       -2     
- Misses      13691    13760      +69     

@vkgnosis vkgnosis enabled auto-merge (squash) May 5, 2022 18:58
@vkgnosis vkgnosis merged commit 04e4731 into main May 5, 2022
@vkgnosis vkgnosis deleted the send-solver-comp branch May 5, 2022 19:01
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants