-
Notifications
You must be signed in to change notification settings - Fork 57
feat: handle app fees #1677
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: master
Are you sure you want to change the base?
feat: handle app fees #1677
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
41229ef
to
4f0cf1f
Compare
4f0cf1f
to
bd3d6ef
Compare
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.
I think the overall approach makes sense to me! There are still some open questions on my side that came up while reviewing. Some might be more of a product question but would be interesting to get your view on it.
api/_dexes/cross-swap-service.ts
Outdated
crossSwap.type === AMOUNT_TYPE.EXACT_INPUT | ||
? destinationSwapQuote.minAmountOut |
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.
This should never be the case because we are inside getCrossSwapQuotesForOutputB2A
which only handles output-amount based flows
api/_dexes/cross-swap-service.ts
Outdated
@@ -1073,6 +1136,7 @@ export async function getCrossSwapQuotesForOutputByRouteA2A( | |||
prioritizedDestinationStrategy.result.bridgeableOutputToken, | |||
routerAddress: | |||
prioritizedDestinationStrategy.result.destinationRouter.address, | |||
destinationOutputAmount: crossSwap.amount, |
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.
We are inside getCrossSwapQuotesForExactInputByRouteA2A
so this needs to be
destinationOutputAmount: crossSwap.amount, | |
destinationOutputAmount: prioritizedDestinationStrategy.indicativeDestinationSwapQuote.minAmountOut, |
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.
appFee is missing here?
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.
Discussed offline about appFee
- We only need to include it in the final bridgeQuote.message
override.
api/_dexes/cross-swap-service.ts
Outdated
@@ -1159,6 +1223,7 @@ export async function getCrossSwapQuotesForOutputByRouteA2A( | |||
prioritizedDestinationStrategy.indicativeDestinationSwapQuote, | |||
bridgeableOutputToken, | |||
routerAddress: destinationRouter.address, | |||
destinationOutputAmount: crossSwap.amount, |
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.
We are inside getCrossSwapQuotesForExactInputByRouteA2A
so this needs to be
destinationOutputAmount: crossSwap.amount, | |
destinationOutputAmount: prioritizedDestinationStrategy.indicativeDestinationSwapQuote.minAmountOut, |
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.
appFee is missing here?
api/swap/_utils.ts
Outdated
// Get USD prices for all tokens and native tokens | ||
const [ | ||
inputTokenPriceUsd, | ||
outputTokenPriceUsd, | ||
originNativePriceUsd, | ||
destinationNativePriceUsd, | ||
] = await Promise.all([ |
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.
From a perf perspective it would be nice if we can pass all prices in as args. So that we do a Promise.all
from within ./approval/_service.ts
alongside one of the async methods so improve latency
api/swap/_utils.ts
Outdated
utils.formatUnits(bridgeFees.totalRelayFee.total, outputToken.decimals) | ||
) * outputTokenPriceUsd; | ||
|
||
const totalFeeUsd = relayerTotalUsd + appFeeUsd; |
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.
We also need to price in the "swap impact". So basically
swapImpactUsd = inputAmountUsd - outputAmountUsd - relayerTotalUsd - appFeeUsd`
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.
Updated!
api/swap/_utils.ts
Outdated
pct: getNormalizedPercentage( | ||
originGasUsd, | ||
params.inputAmount, | ||
inputToken.decimals, | ||
inputTokenPriceUsd | ||
), |
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.
If I think about this one, the pct
doesn't really make sense to me 🤔 Because the origin gas is not priced into the total fees (at least for gasful flows)
api/swap/_utils.ts
Outdated
destinationGas: { | ||
amount: destinationGas.total, | ||
amountUsd: destinationGasUsd, | ||
pct: destinationGas.pct, | ||
}, | ||
relayerCapital: { | ||
amount: relayerCapital.total, | ||
amountUsd: relayerCapitalUsd, | ||
pct: relayerCapital.pct, | ||
}, | ||
relayerTotal: { | ||
amount: bridgeFees.totalRelayFee.total, | ||
amountUsd: relayerTotalUsd, | ||
pct: bridgeFees.totalRelayFee.pct, |
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.
I don't think we can directly reuse the pct values here from the bridge quote. Because in case of origin and/or destination swaps, the swap impact is not priced into the total
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.
Yeah I removed pct
from these for now. I am a little unclear what the percentage is meant to reflect. Is it always w.r.t input amount?
41c8881
to
c6ea809
Compare
454a4f6
to
c9327be
Compare
c6ea809
to
a1d9510
Compare
cf87020
to
af01b7f
Compare
af01b7f
to
02a8289
Compare
02a8289
to
649934c
Compare
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 this looks awesome! I could also send some tx with app fees 💪 There are still some open questions I left on the response format of the fees. We can sync on that offline as well
amountUsd: totalFeeUsd, | ||
pct: totalFeeUsd / inputAmountUsd, | ||
}, | ||
originGas: { |
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.
In the PRD it was specified to also include the respective token information for the components. We can also add this as a fast-follow if you think the required work is a bit too much
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.
So originGas
would include the respective gas tokens of the chain. relayerCapital
, destinationGas
and relayerTotal
in the bridged output token? And app
in the output token
|
||
const bridgeFees = bridgeQuote.suggestedFees; | ||
const relayerCapital = bridgeFees.relayerCapitalFee; | ||
const destinationGas = bridgeFees.relayerGasFee; |
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.
I think we are missing lpFee
api/swap/_utils.ts
Outdated
return { | ||
total: { | ||
amount: totalFeeUsd, | ||
amountUsd: totalFeeUsd, |
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.
We should probably return them as strings? 🤔 To avoid scientific notation but not sure what the best practice is here
api/swap/_utils.ts
Outdated
|
||
return { | ||
total: { | ||
amount: totalFeeUsd, |
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.
Hmm this should probably be in the inputToken?
api/_dexes/cross-swap-service.ts
Outdated
if (appFee.feeAmount.gt(0)) { | ||
bridgeQuote.message = buildExactInputBridgeTokenMessage( | ||
crossSwap, | ||
bridgeQuote.outputAmount, | ||
appFee | ||
); | ||
} |
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.
Isn't this needed even if appFee.feeAmount
is 0? So that the output is updated from crossSwap.amount
to bridgeQuote.amount
?
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.
Yeah good point. Will amend.
649934c
to
96236f4
Compare
96236f4
to
d705281
Compare
appFeePercent
andappFeeRecipient
calculateAppFee
which returns the the fee amount and action to be used by the multicall handlercross-swap-service
for different swap scenarios to apply the app feeTested and documented scenario outputs here: https://docs.google.com/document/d/1YawQNsNY-7I8OXqLImtveve6Y306WTD-cJYmeme7lA8
Closes ACX-4225
Closes ACX-4224