-
Notifications
You must be signed in to change notification settings - Fork 388
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
Improving gas estimates for l2s #53
Conversation
…p/smart-order-router into gas-estimate-improvements
src/routers/alpha-router/gas-models/v3/v3-heuristic-gas-model.ts
Outdated
Show resolved
Hide resolved
src/routers/alpha-router/gas-models/v3/v3-heuristic-gas-model.ts
Outdated
Show resolved
Hide resolved
src/routers/alpha-router/gas-models/v3/v3-heuristic-gas-model.ts
Outdated
Show resolved
Hide resolved
src/routers/alpha-router/gas-models/v3/v3-heuristic-gas-model.ts
Outdated
Show resolved
Hide resolved
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.
Nice! Just nits at this point!
cli/base-command.ts
Outdated
`Total initialized ticks crossed ${v3Route.initializedTicksCrossedList.length}` | ||
); |
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.
Each element in the initializedTIcksCrossedList is the number of ticks crossed for each pool so you need to sum them
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.
Right! Actually these can just be deleted - I just needed these data points when I was manual testing with arbirtrum and optimism (and I was only ever looking at a swap with 1 route).
If you think these are good to print out though in general, I can change it back to summing over all the routes.
cli/base-command.ts
Outdated
@@ -318,5 +320,14 @@ export abstract class BaseCommand extends Command { | |||
estimatedGasUsed: estimatedGasUsed.toString(), | |||
gasPriceWei: gasPriceWei.toString(), | |||
}); | |||
const hops = routeAmounts[0]?.poolAddresses.length; |
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.
Have to sum across all the routeAmounts
Noteable changes:
buildTrade
andbuildSwapMethodParameters
to a separate library so we can access them in the gasModelinitTicksCrossed
,gasUseL1
, andgasCostL1
to the logger so they’re printed at each run of the router (this was to help with manual gas estimation testing but can definitely be removed)