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

improve(API): Apply gas mark up to base fee rather than gas cost #1339

Merged
merged 49 commits into from
Jan 7, 2025

Conversation

nicholaspai
Copy link
Member

@nicholaspai nicholaspai commented Dec 19, 2024

depends on across-protocol/sdk#801

Fixes ACX3600

Expected changes to API

See notes here about impact of bumping the sdk version: https://github.com/across-protocol/frontend/pull/1339/files#r1903169393

If we set the environment variables BASE_FEE_MARKUP and PRIORITY_FEE_MARKUP equal to the existing GAS_MARKUP values today then the gas fees should be quoted the exact same. We can fast follow by reducing these markups.

The main differences will be:

  • 7777777: Zora will be re-added as an OPStack chain which means the l1 gas cost will be added to all estimation. This will make zora quotes a lot more accurate. This is a good thing. Currently, zora quotes are under-estimating gas This is no longer a change since 3568cea was already merged before this

QA Testing

We can test this by querying /api/gas-prices against the same endpoint on the prod deployment. This will show roughly how the gas costs will change with prod.

Comparing against production fee quoting API

Ultimately, what matters is the gasFeeTotal in the /limits end point and we should compare with prod.

  • For WETH:
  • query /limits for every single destination chain once on Prod API and once on Preview API
  • Write down the gasFeeTotal and compare with Preview API
  • Note any differences:
  • If Prod API has higher gas fee total, then preview API will reduce gas fees but we need to make sure these gas fees are still deemed profitable by relayer
  • If preview API has higher gas fee total, then that’s notable as well
  • Can use the preview API from this PR Test: Add gas costs to /gas-prices endpoint #1349 to see the gas cost breakdown from the production API

Comparing against Relayer

Relayer's will only fill deposits that leave enough profits to cover for destination chain gas costs. This means that the relayer's computed tokenGasCosts, including any scaling, should be less than the fee quoting API's tokenGasCosts, otherwise the relayer will consider the API's quotes as unprofitable. A simple way to check this is to check the relayer's log that includes the message "Updated gas cost" (in the "ProfitClient") that will list configs for each chain like:

"at": "ProfitClient",
"message": "Updated gas cost",
...
"1": {
      "nativeGasCost": "131167",
      "tokenGasCost": "961072227772860",
      "gasPrice": "7327088580",
      "scaledTokenGasCost": "276308265484697",
      "gasPadding": "1.15",
      "gasMultiplier": "0.25"
    },
...

I recommend using this relayer PR across-protocol/relayer#1969 to print the above logs and compare with the quoted preview API's fees

Copy link

vercel bot commented Dec 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
app-frontend-v3 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 7, 2025 6:57pm
sepolia-frontend-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 7, 2025 6:57pm

Copy link
Contributor

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

We should let integrators know about this prior to rolling out. This will change expected fees

api/_utils.ts Outdated Show resolved Hide resolved
api/_utils.ts Outdated
: DEFAULT_GAS_MARKUP;
return (
1 +
(sdk.utils.chainIsOPStack(Number(chainId))
Copy link
Member Author

Choose a reason for hiding this comment

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

One difference here. In the current multiplier use case, the SDK adds the gasMarkup to 1 here.

Whereas, the new SDK change I propose assumes that the passed in multiplier is complete already and doesn't need to be added to anything

@@ -1,31 +1,139 @@
import { VercelResponse } from "@vercel/node";
import {
buildDepositForSimulation,
Copy link
Member Author

Choose a reason for hiding this comment

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

This file has a lot of changes but its an API endpoint we don't depend on in production as its mostly here to compare against the current existing API to compare the expected gas total changes. The main changes are:

  • Adding gas costs (native and token gas cost) to the API response
  • breaking down gas price by base fee and priority fee

api/limits.ts Outdated
@@ -194,6 +194,8 @@ const handler = async (
tokenPriceNative,
relayer,
gasUnits,
// !!gas price should be defined and passed into getRelayerFeeDetails so we don't recompute using default
Copy link
Member Author

@nicholaspai nicholaspai Jan 4, 2025

Choose a reason for hiding this comment

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

I think its important to highlight the importance of not letting gasPrice be undefined here, otherwise the effects of the baseFeeMultiplier, based on the developer-set GAS_MARKUP will be discarded. We could even add an assert here...

package.json Outdated
@@ -7,7 +7,7 @@
"@across-protocol/constants": "^3.1.24",
"@across-protocol/contracts": "^3.0.19",
"@across-protocol/contracts-v3.0.6": "npm:@across-protocol/[email protected]",
"@across-protocol/sdk": "^3.3.27",
Copy link
Member Author

Choose a reason for hiding this comment

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

This SDK change has two important changes that will materially impact the gasFeeTotal value returned in the API response of the /limits (and /suggested-fees) endpoint:

  • Zora chain gets re-added back to OP_STACK family which means the fees will go up since OPStack chains add an L1GasCost component
  • GAS_MARKUP will be applied to base fee, not total token gas cost, which will cut many gas fee totals down significantly because base fees are much smaller than priority fees on most chains, for example most OP Stack chains

gasPrice?: sdk.utils.BigNumberish
tokenPrice: number,
relayerAddress: string,
gasPrice: sdk.utils.BigNumberish,
Copy link
Member Author

Choose a reason for hiding this comment

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

Made gasPrice, tokenPrice and relayerAddress all required since they're never set undefined in this repo and its better to be more constricting on this type where possible

gasPrice
gasPrice,
gasCosts?.nativeGasCost,
gasCosts?.tokenGasCost
Copy link
Member Author

Choose a reason for hiding this comment

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

In other words, the change here is: load both nativeGasCost and tokenGasCost from the SDK and pass them both here when computing gasFeePercent. This is useful because the function getCachedFillGasUsage() call uses the opStackL1GasCostMultiplier when computing the tokenGasCost which will give us more configuration options when quoting gas costs.

We now can play with the following configs:

  • baseFeeMultiplier set in latestGasPriceCache
  • priorityFeeMultiplier set in latestGasPriceCache
  • opStackL1GasCostMultiplier set in getCachedFillGasUsage

We take the resultant gas costs and gas prices and pass them into relayerFeeDetails which simply does some simple multiplication and division to return the fee percentages. There should be no more RPC queries in this getRelayerFeeDetails() call

Copy link
Contributor

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

I tested the endpoint and ran the FE - the values look to be expected.

@nicholaspai nicholaspai merged commit f9b431d into master Jan 7, 2025
9 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