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

dex: Swap alt fee tokens for native fee token #4643

Merged
merged 5 commits into from
Jun 25, 2024
Merged

dex: Swap alt fee tokens for native fee token #4643

merged 5 commits into from
Jun 25, 2024

Conversation

hdevalence
Copy link
Member

Describe your changes

Moves fee handling out of the Transaction's ActionHandler and into
the component, which accumulates fees (in any token), and adds hooks to the DEX
thatadd chain-submitted swaps for any non-native fee tokens into the native
token. These are then accumulated back into the fee component.

The base fee and tip are tracked separately through this whole process, in
order to support the intended behaviour (the base fee is burned to account for
the chain's resource use, the tip is sent to the proposer to encourage them
to include transactions in fee priority order). However, tip handling is not
currently implemented, so both are burned. Later, the funding/distribution
components should extract the tip and send it to the proposer's funding
streams. However, until a robust fee market develops, this form of proposer
incentivization can be deferred. The accounting will be there for it when it
arises.

Issue ticket number and link

#4328

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    consensus-breaking but not state-breaking, no migrations necessary

@hdevalence
Copy link
Member Author

Testing:

  • Exercise on a devnet and check that behavior works
  • Add state machine tests that a (non-DEX, say ordinary transfer) tx that pays an alt fee token causes a swap to occur, and that the resulting data is as expected
  • Add DEX VCB tests that check VCB works as expected when there are multiple swaps per block.

Copy link
Member

@erwanor erwanor left a comment

Choose a reason for hiding this comment

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

Left a few comments and ACK, will test this later (pairing on testing would be cool if there are takers)

crates/core/component/fee/src/component.rs Show resolved Hide resolved
crates/core/component/dex/src/component/dex.rs Outdated Show resolved Hide resolved
@zbuc
Copy link
Member

zbuc commented Jun 19, 2024

Left a few comments and ACK, will test this later (pairing on testing would be cool if there are takers)

LGTM overall, I have some bandwidth to write some state machine tests

@zbuc
Copy link
Member

zbuc commented Jun 19, 2024

What should happen if there isn't enough liquidity to perform a swap for the alternative fee token? Currently it will be burned, which seems fine. What about when the funding component is implemented; will tips be paid out in alternative assets if there isn't liquidity available to swap them?

@erwanor
Copy link
Member

erwanor commented Jun 20, 2024

I'm working on this branch, and then will post updates on testing

@erwanor erwanor marked this pull request as draft June 20, 2024 16:56
@erwanor erwanor added consensus-breaking breaking change to execution of on-chain data A-dex Area: Relates to the dex A-fees Area: Relates to transaction fees protobuf-changes Makes changes to the protobuf definitions. labels Jun 21, 2024
hdevalence and others added 3 commits June 23, 2024 16:42
This commit moves fee handling out of the Transaction's ActionHandler and into
the component, which accumulates fees (in any token), and adds hooks to the DEX
thatadd chain-submitted swaps for any non-native fee tokens into the native
token. These are then accumulated back into the fee component.

The base fee and tip are tracked separately through this whole process, in
order to support the intended behaviour (the base fee is burned to account for
the _chain_'s resource use, the tip is sent to the proposer to encourage them
to include transactions in fee priority order). However, tip handling is not
currently implemented, so both are burned.  Later, the funding/distribution
components should extract the tip and send it to the proposer's funding
streams. However, until a robust fee market develops, this form of proposer
incentivization can be deferred. The accounting will be there for it when it
arises.
@erwanor erwanor marked this pull request as ready for review June 24, 2024 04:21
@erwanor
Copy link
Member

erwanor commented Jun 24, 2024

Tested on a devnet earlier today, I was able to observe:

  • alt asset fee burn against an empty dex
  • base fee swap correctly calculated
  • base fee + tip swap correctly calculated
  • corresponding VCB debit/credits that match pre-swap / post-swap values

I am close to done with a SM test that prove that a multiswap block is accounted for correctly by the VCB mechanism. #4645 is a WIP with SM test of a spend transaction generating alt fees that get swapped.

Copy link
Member

@erwanor erwanor left a comment

Choose a reason for hiding this comment

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

We are still working on assurance. However, for the sake of not blocking release readiness I am in favor of merging this as-is once it has collected an additional ACK. We commit to following-up with application tests shortly.

@erwanor erwanor changed the title Swap alt fee tokens for native fee token dex: Swap alt fee tokens for native fee token Jun 24, 2024
@TalDerei
Copy link
Collaborator

TalDerei commented Jun 24, 2024

We are still working on assurance. However, for the sake of not blocking release readiness I am in favor of merging this as-is once it has collected an additional ACK. We commit to following-up with application tests shortly.

I haven't sufficiently tested to ACK, but I support optimistically merging if you have and I'll do more testing later.

Comment on lines +175 to +187
// Obtain the base fee and tip amounts in the native token, discarding any unfilled amounts.
let (swapped_base, swapped_tip) = if pair.asset_1() == *asset_id {
// If `asset_id` is `R_1` we want to pull the other leg of the pair.
(base_output.1, tip_output.1)
} else {
// and vice-versa. `R_1` contains native tokens.
(base_output.0, tip_output.0)
};

// Finally, accumulate the swapped base fee and tip back into the fee component.
// (We already took all the fees out).
state_ref.raw_accumulate_base_fee(Fee::from_staking_token_amount(swapped_base));
state_ref.raw_accumulate_tip(Fee::from_staking_token_amount(swapped_tip));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Obtain the base fee and tip amounts in the native token, discarding any unfilled amounts.
let (swapped_base, swapped_tip) = if pair.asset_1() == *asset_id {
// If `asset_id` is `R_1` we want to pull the other leg of the pair.
(base_output.1, tip_output.1)
} else {
// and vice-versa. `R_1` contains native tokens.
(base_output.0, tip_output.0)
};
// Finally, accumulate the swapped base fee and tip back into the fee component.
// (We already took all the fees out).
state_ref.raw_accumulate_base_fee(Fee::from_staking_token_amount(swapped_base));
state_ref.raw_accumulate_tip(Fee::from_staking_token_amount(swapped_tip));
// Obtain the base fee and tip amounts in the native token, discarding any unfilled amounts.
let ((swapped_base, unswapped_base), (swapped_tip, unswapped_tip)) = if pair.asset_1() == *asset_id {
// If `asset_id` is `R_1` we want to pull the other leg of the pair.
((base_output.1, base_output.0), (tip_output.1, tip_output.0))
} else {
// and vice-versa. `R_1` contains native tokens.
((base_output.0, base_output.1), (tip_output.0, tip_output.1))
};
// Finally, accumulate the swapped base fee and tip back into the fee component.
// (We already took all the fees out).
state_ref.raw_accumulate_base_fee(Fee::from_staking_token_amount(swapped_base));
state_ref.raw_accumulate_tip(Fee::from_staking_token_amount(swapped_tip));
state_ref.raw_accumulate_base_fee(Fee(Value { amount: unswapped_base, asset_id: *asset_id }));
state_ref.raw_accumulate_tip(Fee(Value { amount: unswapped_tip, asset_id: *asset_id }));

What if we also accumulated the unswapped portion?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the goal is to collapse all the user supplied fee amounts to the native token

Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

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

Looks good, main thing I'm unsure about is whether or not we correctly handle the accumulated state of fees at an end of a block:

  • Do we want to keep the unswapped amounts over to the next block, to try swapping again?
  • Do we need to clear the amounts, or is the intention to accumulate them forever, so that the event is the total fees since the start? (I don't think that's the case)

@hdevalence
Copy link
Member Author

On keeping the unwrapped fees: I don't think so. That case shouldn't happen, now or in the future:

  • Now, when there is a governance-controlled set of a few alt fee tokens, we expect those tokens to be on pairs for which there is liquidity. And since the list is known, it creates its own incentive — someone can make a tiny position on each pair with a nearly infinite price knowing that it would act as a position of last resort for any trades.

  • In a future where fee token eligibility is automatic, it would be based on liquidity heuristics anyways.

@hdevalence
Copy link
Member Author

On clearing the accumulated fees: this is supposed to act as a defense measure against value creation bugs, by "taking" the value out before swapping it.

@erwanor
Copy link
Member

erwanor commented Jun 25, 2024

This contains the VCB repro, app test to follow shortly.

@erwanor erwanor merged commit 5e23b95 into main Jun 25, 2024
13 checks passed
@erwanor erwanor deleted the alt-fee-swap_r1 branch June 25, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dex Area: Relates to the dex A-fees Area: Relates to transaction fees consensus-breaking breaking change to execution of on-chain data protobuf-changes Makes changes to the protobuf definitions.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants