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

Handle a case where priorityFee > maxFee in the task loop #655

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

thomaspanf
Copy link
Member

@thomaspanf thomaspanf commented Sep 23, 2024

Automatic transactions in the task loop fail when the user-inputted priorityFee is greater than the oracle based maxFee. This PR addresses this case by setting priorityFee to min(priorityFee, 25% of the oracle based maxFee) when priorityFee > maxFee for transactions in the node task loop.

For example: automatically initializing vote power, defending pDAO proposals, bond reductions and so on.

The change is also applied to watchtower task process-penalties.go

Copy link
Contributor

@jshufro jshufro left a comment

Choose a reason for hiding this comment

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

This is fine but we can do better.

Every t *taskThing has a maxPriorityFee setting. Instead, let's make every t *taskThing have a gasSettings *GasSettings field. Put maxPriorityFee on GasSettings, get rid of GetPriorityFee and instead have a function called something like func (g *GasSettings) ApplyTo(opts *bind.CallOpts) which sets opts.GasFeeCap, opts.GasTipCap, and opts.GasLimit.

Then we get a lot of deduplication out of this PR. Also, add a unit test? it's all arithmetic and logic that is very easy to unit test.

Comment on lines +410 to +412
} else {
return quarterMaxFee
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else {
return quarterMaxFee
}
return quarterMaxFee

Copy link
Contributor

@jshufro jshufro left a comment

Choose a reason for hiding this comment

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

Edit: just realized this is on v1. Fine to merge as-is I guess, but make sure we do this right when we apply it to v2

@0xfornax 0xfornax merged commit 5fab5d3 into master Sep 24, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants