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

[refunder] submitting txs #759

Merged
merged 8 commits into from
Nov 16, 2022
Merged

[refunder] submitting txs #759

merged 8 commits into from
Nov 16, 2022

Conversation

josojo
Copy link
Contributor

@josojo josojo commented Nov 10, 2022

Related to issue: #626

This PR has the logic to make the refunder submit the actual refund tx.

The overall loop goes like this:

  • find submittable uids (done before)
  • measure current gas price
  • check whether there is still an old submission in mempool (on can see it on the nonce) and if found, increase gas price compared to the old tx
  • submit tx
  • if there is an error restart the procedure completely new.

Test Plan

I added some unit tests. E2e test is provided in this PR: #776

@josojo josojo force-pushed the refunderWithNewContract branch 2 times, most recently from f083691 to a1315d4 Compare November 14, 2022 17:40
@josojo josojo marked this pull request as ready for review November 14, 2022 20:35
@josojo josojo requested a review from a team as a code owner November 14, 2022 20:35
@josojo josojo requested a review from fedgiac November 14, 2022 20:35
crates/refunder/src/refund_service.rs Outdated Show resolved Hide resolved
crates/refunder/src/refund_service.rs Outdated Show resolved Hide resolved
crates/refunder/src/refund_service.rs Outdated Show resolved Hide resolved
crates/refunder/src/submitter.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

Still have to look a bit more into the submission logic tomorrow. That stuff tends to be tricky.

crates/refunder/src/refund_service.rs Outdated Show resolved Hide resolved
crates/refunder/src/refund_service.rs Outdated Show resolved Hide resolved
crates/refunder/src/refund_service.rs Outdated Show resolved Hide resolved
crates/refunder/src/refund_service.rs Outdated Show resolved Hide resolved
crates/refunder/src/submitter.rs Outdated Show resolved Hide resolved
crates/refunder/src/submitter.rs Outdated Show resolved Hide resolved
crates/refunder/src/submitter.rs Outdated Show resolved Hide resolved
@vkgnosis vkgnosis mentioned this pull request Nov 16, 2022
crates/refunder/src/submitter.rs Outdated Show resolved Hide resolved
crates/refunder/src/submitter.rs Show resolved Hide resolved
crates/refunder/src/submitter.rs Show resolved Hide resolved
Copy link
Contributor

@fedgiac fedgiac left a comment

Choose a reason for hiding this comment

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

Looks good to me with the exception that an eth-flow order can technically be partially fillable.

crates/refunder/src/refund_service.rs Outdated Show resolved Hide resolved
crates/refunder/src/refund_service.rs Outdated Show resolved Hide resolved
crates/refunder/src/submitter.rs Outdated Show resolved Hide resolved
crates/refunder/src/submitter.rs Outdated Show resolved Hide resolved
crates/refunder/src/submitter.rs Show resolved Hide resolved
crates/refunder/src/submitter.rs Outdated Show resolved Hide resolved
Comment on lines +154 to +158
// The gas price of the refund tx is the current prevailing gas price
// of the web3 gas estimation plus a buffer.
// Since we are using Eip1559 gas specification,
// we will only pay the buffer if it is used
let mut new_max_fee_per_gas = max_fee_per_gas * GAS_PRICE_BUFFER_FACTOR;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we do it this way in services too? I feel like we could simplify our code in the same way.

Copy link
Contributor

@fedgiac fedgiac left a comment

Choose a reason for hiding this comment

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

Nice!

crates/refunder/src/ethflow_order.rs Outdated Show resolved Hide resolved
crates/refunder/src/ethflow_order.rs Outdated Show resolved Hide resolved
@josojo josojo enabled auto-merge (squash) November 16, 2022 15:33
@josojo josojo merged commit a84f2d6 into main Nov 16, 2022
@josojo josojo deleted the refunderWithNewContract branch November 16, 2022 15:38
@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 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.

4 participants