-
Notifications
You must be signed in to change notification settings - Fork 57
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
feat(profitability): Remove hardcoded min revenue map and add logic to consider gas cost #212
Changes from all commits
7198daa
9395e48
a9b0ab4
e3233ad
ed9d47f
fe1f6fd
a9e9c30
e52a467
cd44aae
2e7b298
ca839bd
d232b02
e850af8
0d94335
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,36 +2,26 @@ import { BigNumber, winston, toBNWei, toBN, assign } from "../utils"; | |
import { HubPoolClient } from "."; | ||
import { Deposit, L1Token } from "../interfaces"; | ||
import { Coingecko } from "@uma/sdk"; | ||
import * as _ from "lodash"; | ||
|
||
// Define the minimum revenue, in USD, that a relay must yield in order to be considered "profitable". This is a short | ||
// term solution to enable us to avoid DOS relays that yield negative profits. In the future this should be updated | ||
// to actual factor in the cost of sending transactions on the associated target chains. | ||
const chainIdToMinRevenue = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Context: https://risklabs.slack.com/archives/D03GKFYCDJ7/p1657999254013629. tldr; is this hardcoded limit is filtering out a lot of small deposits with profits of < $1. When running with this previous logic, my relayer was skipping 9/10 deposits. We'll follow up with data analysis to understand profitability of small deposits. |
||
// Mainnet and L1 testnets. | ||
1: toBNWei(10), | ||
4: toBNWei(10), | ||
5: toBNWei(10), | ||
42: toBNWei(10), | ||
// Rollups/L2s/sidechains & their testnets. | ||
10: toBNWei(1), | ||
69: toBNWei(1), | ||
288: toBNWei(1), | ||
28: toBNWei(1), | ||
42161: toBNWei(1), | ||
137: toBNWei(1), | ||
80001: toBNWei(1), | ||
// We use wrapped ERC-20 versions instead of the native tokens such as ETH, MATIC for ease of computing prices. | ||
export const WMATIC = "0x0d500B1d8E8eF31E21C99d1Db9A6444d3ADf1270"; | ||
nicholaspai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const GAS_TOKEN_BY_CHAIN_ID = { | ||
1: "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2", // WETH | ||
10: "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2", // WETH | ||
137: WMATIC, | ||
288: "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2", // WETH | ||
42161: "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2", // WETH | ||
}; | ||
// TODO: Make this dynamic once we support chains with gas tokens that have different decimals. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope there is no world where a chain has a non-18 decimal gas token but sadly this will probably be the case |
||
const GAS_TOKEN_DECIMALS = 18; | ||
|
||
export class ProfitClient { | ||
private readonly coingecko; | ||
protected tokenPrices: { [l1Token: string]: BigNumber } = {}; | ||
private unprofitableFills: { [chainId: number]: { deposit: Deposit; fillAmount: BigNumber }[] } = {}; | ||
|
||
constructor( | ||
readonly logger: winston.Logger, | ||
readonly hubPoolClient: HubPoolClient, | ||
readonly relayerDiscount: BigNumber = toBNWei(0) | ||
) { | ||
constructor(readonly logger: winston.Logger, readonly hubPoolClient: HubPoolClient) { | ||
this.coingecko = new Coingecko(); | ||
} | ||
|
||
|
@@ -41,7 +31,7 @@ export class ProfitClient { | |
|
||
getPriceOfToken(token: string) { | ||
if (!this.tokenPrices[token]) { | ||
this.logger.warn({ at: "ProfitClient", message: `Token ${token} not found in state. Using 0` }); | ||
this.logger.debug({ at: "ProfitClient", message: `Token ${token} not found in state. Using 0` }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be noisy when coingecko apis keep failing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm good with this, coingecko API seems to fail a lot. Any ideas for a fallback strategy? |
||
return toBN(0); | ||
} | ||
return this.tokenPrices[token]; | ||
|
@@ -55,12 +45,7 @@ export class ProfitClient { | |
this.unprofitableFills = {}; | ||
} | ||
|
||
isFillProfitable(deposit: Deposit, fillAmount: BigNumber) { | ||
if (toBN(this.relayerDiscount).eq(toBNWei(1))) { | ||
this.logger.debug({ at: "ProfitClient", message: "Relayer discount set to 100%. Accepting relay" }); | ||
return true; | ||
} | ||
|
||
isFillProfitable(deposit: Deposit, fillAmount: BigNumber, gasUsed: BigNumber) { | ||
if (toBN(deposit.relayerFeePct).eq(toBN(0))) { | ||
this.logger.debug({ at: "ProfitClient", message: "Deposit set 0 relayerFeePct. Rejecting relay" }); | ||
return false; | ||
|
@@ -69,10 +54,15 @@ export class ProfitClient { | |
const tokenPriceInUsd = this.getPriceOfToken(l1Token); | ||
const fillRevenueInRelayedToken = toBN(deposit.relayerFeePct).mul(fillAmount).div(toBN(10).pow(decimals)); | ||
const fillRevenueInUsd = fillRevenueInRelayedToken.mul(tokenPriceInUsd).div(toBNWei(1)); | ||
|
||
// Consider gas cost. | ||
const gasCostInUsd = gasUsed | ||
.mul(this.getPriceOfToken(GAS_TOKEN_BY_CHAIN_ID[deposit.destinationChainId])) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we need to compute the current gas price to take into account fluctuations in relayer costs? I think most chains have gas costs that can vary by up to 10x week-to-week, so I don't think hardcoding USD revenue will be sufficient to match the dynamic estimations being done by the frontend. Note: maybe I'm missing the point of this PR! Feel free to clarify in the description. I'm just going off of the title. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gasUsed already includes the gas fee (gasUsed = gas cost * gas fee) as passed in from Relayer. Regarding the hardcoded min USD revenue. We'll likely remove them as the first step as we currently have a lot of small deposits with < $1 profit (after gas costs). Depending on external relayer demand, we might or might add back the ability to configure a min profit amount. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm, maybe we're just using different language to describe the same thing. I'm talking about the USD costs, which would amount to gasUsed * gasPrice * gasTokenPrice. I don't think this code takes I think if we want this code to correctly take into account the true cost of relaying a transaction, we need to estimate the gas cost that will be paid. Do you agree with that assessment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what I was talking about. That multiplication you're looking for is done in https://github.com/across-protocol/relayer-v2/pull/212/files#diff-562d70c5c8a2a3e8074947c1f599ce3c57b2abd4cdff462422b5bc9fa4257e9fR76 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, i understand matt's confusion. Its that usually "gas used" in web3/ethers context refers the EVM gas assessed for a smart contract function. Also, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, exactly. Sorry about that @kevinuma. You're completely right. Thanks for the clarification! |
||
.div(toBN(10).pow(GAS_TOKEN_DECIMALS)); | ||
|
||
// How much minimumAcceptableRevenue is scaled. If relayer discount is 0 then need minimumAcceptableRevenue at min. | ||
const revenueScalar = toBNWei(1).sub(this.relayerDiscount); | ||
const minimumAcceptableRevenue = chainIdToMinRevenue[deposit.destinationChainId].mul(revenueScalar).div(toBNWei(1)); | ||
const fillProfitable = fillRevenueInUsd.gte(minimumAcceptableRevenue); | ||
const fillProfitInUsd = fillRevenueInUsd.sub(gasCostInUsd); | ||
const fillProfitable = fillProfitInUsd.gte(toBN(0)); | ||
this.logger.debug({ | ||
at: "ProfitClient", | ||
message: "Considered fill profitability", | ||
|
@@ -81,8 +71,9 @@ export class ProfitClient { | |
tokenPriceInUsd, | ||
fillRevenueInRelayedToken, | ||
fillRevenueInUsd, | ||
minimumAcceptableRevenue, | ||
discount: this.relayerDiscount, | ||
gasUsed, | ||
gasCostInUsd, | ||
fillProfitInUsd, | ||
fillProfitable, | ||
}); | ||
return fillProfitable; | ||
|
@@ -98,26 +89,48 @@ export class ProfitClient { | |
} | ||
|
||
async update() { | ||
const l1Tokens = this.hubPoolClient.getL1Tokens(); | ||
// Clone because we'll be adding WMATIC to the list for logging purposes later below. | ||
const l1Tokens = _.cloneDeep(this.hubPoolClient.getL1Tokens()); | ||
this.logger.debug({ at: "ProfitClient", message: "Updating Profit client", l1Tokens }); | ||
const prices = await Promise.allSettled(l1Tokens.map((l1Token: L1Token) => this.coingeckoPrice(l1Token.address))); | ||
const priceFetches = l1Tokens.map((l1Token: L1Token) => this.coingeckoPrice(l1Token.address)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you considered trying to use the same SDK functionality as the FE to compute the expected relayer fee? One failure mode that we saw in across v1 when we had separate implementations of the expected fee was that our frontend would produce transactions that our bot would then feel were underpriced or vice-versa. This isn't so bad if the bot's estimation is lower than the frontend, but when the frontend estimated lower than the bot, we would end up not relaying the transaction, creating a bad UX and support tickets where people ask when their money will arrive. The strategy that we used was to use the same library to compute the estimated relayer fee in both places and get the FE to charge a configurable percentage more than the bot to ensure that small swings in gas prices between the FE estimation and the bot seeing the deposit wouldn't cause a deposit to get stuck in limbo. Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My next followup is to update the SDK. I think the SDK would need a slightly different implementation for including gas costs in the relayer fee. We currently have a lot of small deposits with single digit in cents of profits. So the SDK seems to be underpricing some deposits. We'll need to add a gas buffer amount in SDK in case of gas spikes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why do you think the computation in the frontend should be different from the relayer? How do you think they should differ, specifically?
Is there anything wrong with profiting a few cents on a small, cheap transaction? For instance, if the gas is $0.001 and the capital required is $0.50, a profit of $0.005 seems pretty reasonable, no?
Agreed, a buffer percentage makes a lot of sense! Hopefully, this could just be a difference in parameterization of the same computation.
Rather than reinventing the wheel, why not start with integrating the SDK implementation that's running in prod and takes into account gas prices and configurable capital costs and try to fit that into this implementation, fixing any bugs or incompatibilities we find along the way? This has the added advantage of ensuring that the results of both computations will match up every step along the way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why is this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think the bot should potentially have a different gas buffer from the SDK. For example when computing relayer fee %, the SDK can use a buffer of 20 gwei on L1 for gas fee buffer (let's say current gas is 20-30 gwei). The relayer, on the other hand, might want to use 30 gwei instead when computing gas cost in profitability, because it wants to absolutely be sure it'd be profitable and not incur any loss due to gas spikes
yes but this thin margin means there's a decent chance of incurring a loss if gas turns against you.
Makes sense. Ideally both SDK and relayer should be using the same formula/calculation but potentially with different gas buffer. I'll update this PR to use the SDK's code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Couldn't the relayer instantiate the SDK with a different There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@nicholaspai agree that this should be a parameterization of the SDK. Rather than it being a GWEI buffer, might be easier for us to make it percentage-based since gas prices work differently on different chains (for instance arbitrum and optimism have two types of gas), so a GWEI offset doesn't really work. On your example @kevinuma, I think we'll ultimately want to make this configurable in both places as a GAS_MULTIPLIER, GAS_MARGIN, or GAS_DISCOUNT (same thing but inverted sign), so each caller can customize it. However, I think we probably want it to be the opposite of your example. In your example, my understanding is that the FE/API would always be underpriced relative to the relayer, so we'd be blocking most relays due to unprofitability. I think we want the UI to be more conservative than the relayer, because we want to avoid users who use our UI or API getting stuck (this is a bad product UX and creates support tickets). So I think, in general, we want GAS_MARGIN in the frontend/api to be greater than the default GAS_MARGIN in the relayer. Note: we can't control how 3rd parties set this, but we'd want them to use the default so they are willing to relay 99.9% of deposits sent through the FE/API. Does that make sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm splitting hairs here a bit, but just because the absolute size and costs are low doesn't mean we have a higher chance of losing money I don't think. Likelihood of loss is dependent on gas price volatility. The amount of money lost on a particular transaction is likely to be lower on a chain that consistently has lower gas prices. For instance, a bot that had no profitability module lost > $1k on a single transaction in Across v1 due to a mainnet gas spike. This size of loss is nearly impossible on a cheap chain, like polygon because gas prices just never get high enough. Either way, I think we're aligned on how this system should work! |
||
// Add WMATIC for gas cost calculations. | ||
priceFetches.push(this.coingeckoPrice(WMATIC, "polygon-pos")); | ||
const prices = await Promise.allSettled(priceFetches); | ||
// Add to l1Tokens after the fetches, so prices and l1Tokens have the same entries, for any error logging later. | ||
l1Tokens.push({ | ||
address: WMATIC, | ||
symbol: "WMATIC", | ||
decimals: 18, | ||
}); | ||
|
||
const errors = []; | ||
for (const [index, priceResponse] of prices.entries()) { | ||
if (priceResponse.status === "rejected") errors.push(l1Tokens[index]); | ||
else this.tokenPrices[l1Tokens[index].address] = toBNWei(priceResponse.value[1]); | ||
} | ||
if (errors.length > 0) { | ||
let mrkdwn = "The following L1 token prices could not be fetched:\n"; | ||
let warningMrkdwn = ""; | ||
let debugMrkdwn = ""; | ||
errors.forEach((token: L1Token) => { | ||
mrkdwn += `- ${token.symbol} Not found. Using last known price of ${this.getPriceOfToken(token.address)}.\n`; | ||
const lastPrice = this.getPriceOfToken(token.address); | ||
if (lastPrice && lastPrice.eq(toBN(0))) { | ||
warningMrkdwn += `- ${token.symbol} Not found.\n`; | ||
} else { | ||
debugMrkdwn += `- ${token.symbol} Not found. Using last known price of ${debugMrkdwn}.\n`; | ||
} | ||
}); | ||
this.logger.warn({ at: "ProfitClient", message: "Could not fetch all token prices 💳", mrkdwn }); | ||
|
||
if (warningMrkdwn) { | ||
warningMrkdwn = "The following L1 token prices could not be fetched:\n" + warningMrkdwn; | ||
this.logger.warn({ at: "ProfitClient", message: "Could not fetch all token prices 💳", mrkdwn: warningMrkdwn }); | ||
} else { | ||
this.logger.debug({ at: "ProfitClient", message: "Could not fetch all token prices", mrkdwn: debugMrkdwn }); | ||
} | ||
} | ||
this.logger.debug({ at: "ProfitClient", message: "Updated Profit client", tokenPrices: this.tokenPrices }); | ||
} | ||
|
||
private async coingeckoPrice(token: string) { | ||
return await this.coingecko.getCurrentPriceByContract(token, "usd"); | ||
private async coingeckoPrice(token: string, platform_id?: string) { | ||
return await this.coingecko.getCurrentPriceByContract(token, "usd", platform_id); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,6 +63,23 @@ export async function willSucceed( | |
} | ||
} | ||
|
||
export async function estimateGas( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think it might be clearer if we called this estimateGasCost, since estimateGas is the name of the method that is usually used to report the amount of gas rather than the amount of tokens used for gas. |
||
transaction: AugmentedTransaction | ||
): Promise<{ transaction: AugmentedTransaction; succeed: boolean; reason: string; gasUsed: BigNumber }> { | ||
try { | ||
const args = transaction.value ? [...transaction.args, { value: transaction.value }] : transaction.args; | ||
const [gasUsed, gasFee] = await Promise.all([ | ||
transaction.contract.estimateGas[transaction.method](...args), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing we need to be careful about when estimating gas on this transaction is that it will revert if the estimation is sent from a wallet that either doesn't have enough of the currency or has not approved the contract to pull that token. So we need to make sure that this defaults to using a wallet that will always work. Is it possible to get here if the relayer doesn't have the balance to run this transaction? Is the relayer address always used as the default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be safe we can use the address hardcoded here in the sdk-v2 repo |
||
transaction.contract.provider.getGasPrice(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you get this gas price once per relayer loop and then re use it for all invocations of estimateGas? It seems like this would be a low risk way to reduce requests to the RPC. Also, is provider.getGasPrice the same method that is used by runTransaction to get gas price? Do we not use a GasEstimatorClient? I only ask because we should be consistent with how we query gas price There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also I just realized, why don't you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It basically gives you the result of eth_gasPrice from the node. The rough algorithm used to compute this is here. I think this is pretty reasonable. I think the SDK does something similar for some chains. The issue I see with this approach is that I'm not sure it will give reasonable results for all L2s. Optimism has a custom provider that has a special method to compute the estimated gas costs because optimism has multiple types of gas (see the SDK implementation). For the other chains, this approach should mostly work I think. We mostly follow these rules in the SDK, specific to each chain, but there are definitely areas to improve the estimation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regardless of how |
||
]); | ||
|
||
return { transaction, succeed: true, reason: null, gasUsed: gasUsed.mul(gasFee) }; | ||
} catch (error) { | ||
console.error(error); | ||
return { transaction, succeed: false, reason: error.reason, gasUsed: toBN(0) }; | ||
} | ||
} | ||
|
||
export function getTarget(targetAddress: string) { | ||
try { | ||
return { targetAddress, ...getContractInfoFromAddress(targetAddress) }; | ||
|
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.
Unused vars