Charge accurate fees #418
Charge accurate fees #418
Conversation
…ctuation in hardhat)
There was a problem hiding this comment.
Tested out your script and looks like everything is working. I also really like the events you added 💯 .
The comments I added are non-blocking, they are just around being more descriptive. I think some added comments describing some of the logic and maybe breaking some things out into variables could be useful for readability.
| ].join(" ")); | ||
| } | ||
|
|
||
| await (await VerificationGateway__factory.connect( |
There was a problem hiding this comment.
nit but this double await with a .wait() at the end is confusing to read. Could we break this up into multiple variables/lines?
There was a problem hiding this comment.
This is a pretty common pattern, so my vote is to keep it compact like this. Might be worth putting this to the team though.
There was a problem hiding this comment.
No worries, if it's common to do it like this I'm cool with it 👍
| return { | ||
| maxFeePerGas: previousBaseFee | ||
| .add( | ||
| previousBaseFee.mul(env.PREVIOUS_BASE_FEE_PERCENT_INCREASE).div(100), |
There was a problem hiding this comment.
Might be nice to give this a variable for readability. ex:
const feeIncrease = previousBaseFee.mul(env.PREVIOUS_BASE_FEE_PERCENT_INCREASE).div(100);
| assert(previousBaseFee !== null && previousBaseFee !== nil); | ||
|
|
||
| return { | ||
| maxFeePerGas: previousBaseFee |
There was a problem hiding this comment.
Could be useful to have a comment that mentions why we set maxFeePerGas to the previousBaseFee + a fee increase + a priority fee per gas.
| return filteredRows; | ||
| } | ||
|
|
||
| async #pickBest(rows: BundleRow[]) { |
There was a problem hiding this comment.
Could we add a comment here explaining how we pick the best?
| return { | ||
| aggregateBundle: nil, | ||
| includedRows: [], | ||
| bundleOverheadCost: bundleOverheadGas.mul( |
There was a problem hiding this comment.
My brain is moving a little slow 😅 why is this the bundleOverheadGas * maxFeePerGas ?
There was a problem hiding this comment.
It's just the basic gas mechanism - transactions consume gas, and you pay a fee per unit of gas, therefore the cost is the amount of gas times the fee paid per gas.
|
|
||
| return rows[bestExcessFeeIndex]; | ||
| } | ||
|
|
||
| async #augmentAggregateBundle( |
There was a problem hiding this comment.
I know you didn't edit this, but it might be nice to note what are we augmenting
|
Also, I really like your description of the PR and the description in the linked issue. Do you think we should add any of that explanation to the Readme? |
Cheers. Fixed in #438. |
What is this PR doing?
Changes the way we make bundles and charge for them, so that the aggregator has a good handle on the fees it needs to be charging and is able to turn a profit without charging more than necessary.
There's a fair bit of detail in the issue about this: #415.
Previously
estimateFeewould report the fee required to submit the user's bundle in isolation, which defeats the whole purpose of saving money by aggregating with other users. This has been fixed so that the required fee reported includes only the marginal cost of the bundle and a configurable share of the bundle's overhead (basically the block space for the single signature and validating it).When forming an aggregate bundle, we used to put all available bundles together and check that it satisfies our
estimateFeecalculation as a whole. Then, if it didn't work, we'd do a binary search to figure out how many of those bundles we could include, exclude the next one, and then try to add more recursively.The binary search method had the advantage of processing bundles in order, allowing for things like chaining dependent bundles together (e.g. consecutive nonces). However, this relies on the idea that all the bundles are supposed to be self-sufficient (e.g. even just one bundle pays enough to cover itself), which is wrong. I decided to simplify this by evaluating bundles individually to make sure they satisfy
estimateFee, and then just checking once that the overall bundle is profitable (or just letting it through regardless ifALLOW_LOSSESistrue).How can these changes be manually tested?
Use
./manualTests/mintNViaAggregator.ts <N>with different values of N and check the profitability output is acceptable.For example, here's some output I got from the aggregator with
BREAKEVEN_OPERATION_COUNT=4.5and./manualTests/mintNViaAggregator 5:When
Nis greater thanBREAKEVEN_OPERATION_COUNT, bundles should be profitable. Note that due to fluctuations,mintNViaAggregatoris additionally adding 10% to the fee, so smaller values of N may be profitable too.You can also check that if
ALLOW_LOSSESisfalse, then the aggregator generally won't submit unprofitable bundles, and will at least not submit bundles it expects to make losses on (expectedFeeshould be higher thanexpectedMaxCost(both of these may deviate from their actual values)).Does this PR resolve or contribute to any issues?
Resolves #415
Checklist
Guidelines
resolve conversationbutton is for reviewers, not authors