-
Notifications
You must be signed in to change notification settings - Fork 577
fix: simplify fee strategies to run promises internally #19225
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
base: next
Are you sure you want to change the base?
Conversation
| const strategyPromisesArr = []; | ||
| for (const [key, promise] of Object.entries(strategyPromises)) { | ||
| // Get strategy promise factories for priority fee calculation | ||
| const strategyFactories = CurrentStrategy.getRequiredPromiseFactories(this.client, { isBlobTx }); |
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.
The way these strategies are currently used and implemented requires quite a bit of ugly non-typesafe code and back and forth between the stratgey and the caller. As I understand it, it's because the strategies don't have consistent interfaces. But in each case from what I can see, the flow is:
const strategyResults: Array<unknown> = await Promise.all(strategy.createSomePromises);
// ..... some other code goes here
const finalResult = strategy.calculate(strategyResults as any, someOtherArgs );
Ultimately I think all (both) strategies result in a feePerGas value. So why don't we just push everything to the strategy?
const feePerGas = await strategy.CalculateFee(this.client, { isBlobTx }); // This does everything required
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.
The latestBlock and blobBaseFee data are both retrieved from the instance of this.client so I'm not sure what else is even needed by the strategy.
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.
It does feel a bit overengineered but the reasoning was:
- define the promises separate to the strategy so that they can all get called in
l1_tx_utilsat the same time as the standard calls,latestBlock&blobBaseFee:
const [latestBlockResult, blobBaseFeeResult, ...strategyResults] = await Promise.allSettled([
latestBlockPromise,
blobBaseFeePromise ?? Promise.resolve(0n),
...strategyPromisesArr,
]);
This way we fire all RPC requests at once and save time
- Using generic promises because other strategies may involve requests that are not done to an L1 RPC. e.g. we discussed about calculating profitability of the proposer which would involve fetching aztec token & ethereum prices
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.
Can't this just be done inside the strategy though?
const [latestBlockResult, blobBaseFeeResult, ...strategyResults] = await Promise.allSettled([
latestBlockPromise,
blobBaseFeePromise ?? Promise.resolve(0n),
...strategyPromisesArr,
]);
We can define a config/args type that contains every dependency needed by any strategy and pass it to all strategies. That way any given strategy is free to use what they need.
small fix to return factories rather than triggered promises in fee strategies (
tryTwicewas not working properly)