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

adding dynamic swap fee to fx pools #1101

Open
wants to merge 10 commits into
base: v3-canary
Choose a base branch
from

Conversation

gmbronco
Copy link
Collaborator

@gmbronco gmbronco commented Oct 24, 2024

closes #802

Copy link

changeset-bot bot commented Oct 24, 2024

🦋 Changeset detected

Latest commit: 301f5cd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
backend Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Collaborator

@franzns franzns 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 think about how a negative fee and APR affects our UI and if this is what we want to show

// Replica of the subgraph logic:
// https://github.com/balancer/balancer-subgraph-v2/blob/60453224453bd07a0a3a22a8ad6cc26e65fd809f/src/mappings/vault.ts#L551-L564
if (swap.poolId.poolType === 'FX') {
const USDC_ADDRESS = '0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48';
Copy link
Collaborator

Choose a reason for hiding this comment

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

How can we hardcode this? Isnt this different for each chain?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we only compare against USDC? No other FOREX such as EURC?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like all the markets are against USDC, but true that it needs to be done for other chains as well.

Copy link

@schystz schystz Oct 31, 2024

Choose a reason for hiding this comment

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

Hi @franzns, @gmbronco, would it be possible to add a quoteToken property on the Pool model? This new prop will hold the quote token address, whether USDC, USDC.e, or any token.

This is how we assign the quoteToken value in our balancer-subgraph fork:

function handleNewFXPool(event: ethereum.Event, permissionless: boolean): void {
...
  pool.quoteToken = getFXPoolQuoteToken(poolAddress);
...
}

function getFXPoolQuoteToken(poolAddress: Address): Bytes {
  let poolContract = FXPool.bind(poolAddress);
  // FXPool **always** holds the quote token in the second position
  let call = poolContract.try_derivatives(BigInt.fromI32(1));
  if (call.reverted) {
    log.error('Failed to get quote token for FXPool: {}', [poolAddress.toHexString()]);
    return stringToBytes('0x');
  }
  return call.value;
}

We then refer to quoteToken instead of hardcoded USDC addresses to determine base and quote:

let isTokenInBase = tokenOutAddress === pool.quoteToken;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @franzns, @gmbronco, would it be possible to add a quoteToken property on the Pool model? This new prop will hold the quote token address, whether USDC, USDC.e, or any token.

This is how we assign the quoteToken value in our balancer-subgraph fork:

function handleNewFXPool(event: ethereum.Event, permissionless: boolean): void {
...
  pool.quoteToken = getFXPoolQuoteToken(poolAddress);
...
}

function getFXPoolQuoteToken(poolAddress: Address): Bytes {
  let poolContract = FXPool.bind(poolAddress);
  // FXPool **always** holds the quote token in the second position
  let call = poolContract.try_derivatives(BigInt.fromI32(1));
  if (call.reverted) {
    log.error('Failed to get quote token for FXPool: {}', [poolAddress.toHexString()]);
    return stringToBytes('0x');
  }
  return call.value;
}

We then refer to quoteToken instead of hardcoded USDC addresses to determine base and quote:

let isTokenInBase = tokenOutAddress === pool.quoteToken;

Yes, we should definitely do that. We already have pool type specific data where we can add the quote token. Is the quote token immutable?

Copy link

Choose a reason for hiding this comment

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

Yes, the quote token is immutable for FX pools

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the quote token to our pool type specifc data set

}
}

feeUSD = String(feeFloatUSD);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think we talked about the option of a possible negative APR. Would that mean that his fee might be negative? What are the downstream effects if so?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is there upside / downside? i assume it's just a property of FX pools. unless you mean if some other parts of the app will need to be updated. that i'm not sure. Is there anything that depends on the apr?

@schystz
Copy link

schystz commented Oct 31, 2024

Hi @gmbronco, don't we also need to update how the fees24h is computed in pool.snapshot-service.ts? (note: this file seems to exist in two places)

The code assumes a static pool swap fee, but FX pools have a dynamic swap fee

fees24h: volume24h * parseFloat(poolAtBlock.swapFee)

I'm thinking something like this might work:

fees24h: parseFloat(pool.totalSwapFee) - parseFloat(pool24hAgo.totalSwapFee)

@franzns
Copy link
Collaborator

franzns commented Oct 31, 2024

Hi @gmbronco, don't we also need to update how the fees24h is computed in pool.snapshot-service.ts? (note: this file seems to exist in two places)

The code assumes a static pool swap fee, but FX pools have a dynamic swap fee

fees24h: volume24h * parseFloat(poolAtBlock.swapFee)

I'm thinking something like this might work:

fees24h: parseFloat(pool.totalSwapFee) - parseFloat(pool24hAgo.totalSwapFee)

Good catch. We'll need to think about that as we'll change the snapshot generation not to rely on subgraph pricing anymore. We can calculate fees24h based on the swap events for example which would be more accurate.

@schystz
Copy link

schystz commented Oct 31, 2024

Hi @gmbronco, don't we also need to update how the fees24h is computed in pool.snapshot-service.ts? (note: this file seems to exist in two places)
The code assumes a static pool swap fee, but FX pools have a dynamic swap fee

fees24h: volume24h * parseFloat(poolAtBlock.swapFee)

I'm thinking something like this might work:

fees24h: parseFloat(pool.totalSwapFee) - parseFloat(pool24hAgo.totalSwapFee)

Good catch. We'll need to think about that as we'll change the snapshot generation not to rely on subgraph pricing anymore. We can calculate fees24h based on the swap events for example which would be more accurate.

Great, that makes sense to me!

@gmbronco gmbronco force-pushed the adding-dynamic-swap-fee-to-fx-pools branch from dafb09d to 08b5c00 Compare November 20, 2024 17:16
@gmbronco gmbronco force-pushed the adding-dynamic-swap-fee-to-fx-pools branch from 08b5c00 to dbfac77 Compare November 22, 2024 09:39
@gmbronco
Copy link
Collaborator Author

@franzns this one is ready apart from the snapshot's fees24h, which we don't have in subgraph ATM. (added a separate ticket for it here: #1182)

Comment on lines +8 to +18
const update = async (data: { id: string; chain: Chain; typeData: any }[]) => {
// Update the pool type data
const updates = data.map(({ id, chain, typeData }) =>
prisma.prismaPool.update({
where: { id_chain: { id, chain } },
data: { typeData },
}),
);

await prismaBulkExecuteOperations(updates, false);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont think this works as a general approach. Some pool types will have mutable and immutable type data, meaning some will be synced at creation via subgraph and some will be synced via on-chain in regular intervals.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how does it matter? do you mean to avoid unnecessary update calls?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It actually doesnt really here as you handle FX pools outside of the "update type data flow".

Comment on lines -47 to +48
valueUSD: String((feeToken?.price || 0) * parseFloat(swap.payload.fee.amount)),
valueUSD: String(feeValueUSD > 0 ? feeValueUSD : swap.payload.fee.valueUSD),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you set it to swap.payload.fee.valueUSD because that is still 0 at this time? Think this is a but bogus, just set to 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a passthrough for a default USD price coming from the subgraph, in case there is no pricing in the DB. It's set earlier and it important to the FX fee, because it's set based on the subgraph data of latestFx rates.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah gotcha, this is v2. This is just used as fallback so its fine 👍

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.

Support FX pools (apr, swap fee)
3 participants