Skip to content

feat(trading-proto-upgrade): fees fixes among other things#2109

Draft
mariocynicys wants to merge 44 commits intodevfrom
proto-upgrade
Draft

feat(trading-proto-upgrade): fees fixes among other things#2109
mariocynicys wants to merge 44 commits intodevfrom
proto-upgrade

Conversation

@mariocynicys
Copy link
Collaborator

The main feat here is taker payment spend fee negotiation (I guess we will refactor namings in the swaps v2 code and rename this to taker payment fee/funding spend fee):

The negotiation goes as follows:

  • Taker and maker each decide how much this fee should be at the beginning of the swap.
  • The taker will send the maker their proposed fee during negotiation,
    if the maker deems the fee as low enough (less than 90% of the maker's own calculated fee),
    they will refuse to trade before the trade starts. Otherwise, they
    will continue and use this fee for validation later.
  • The maker will validate that the taker has accounted for this fee in
    the funding transaction.
  • The taker will validate that the taker payment preimage (generated by the maker)
    uses the negotiated fee or greater (greater number will deduct from the maker's trading volume anyway).
    If not, the taker will stop the trade since the maker is clearly stealing funds that isn't his
    which might affect the completion of the swap if the fee is low.

Proto v2 issue pool:

  • was already fixed:
    • immediate taker refund using S2
    • accept_only_from our order peer.
  • fixed in this PR:
    • use a u8 fork id
    • negotiate funding spend fee and let taker add it to the funding tx
  • still missing:
    • research: concurrent conf
    • recover_funds_for_swap rpc for swaps v2
    • investigation of why test_taker_completes_swap_after_restart is flaky
    • other refactors

Fixmes & discuss comments are intended to be covered in the PR.

@shamardy shamardy requested review from laruh and shamardy May 6, 2024 15:00
@shamardy shamardy changed the title Proto upgrade fixups feat(trading-proto-upgrade): fees fixes among other things May 6, 2024
@shamardy
Copy link
Collaborator

shamardy commented May 6, 2024

@mariocynicys please add label for if this is under review or in progress.

@laruh
Copy link

laruh commented May 7, 2024

@mariocynicys please merge dev to feature branch as it contains build fixes
https://github.com/KomodoPlatform/komodo-defi-framework/actions/runs/8965111556/job/24617995991?pr=2109#step:8:574

error[E0463]: can't find crate for `core`
  |
  = note: the `x86_64-apple-darwin` target may not be installed
  = help: consider downloading the target with `rustup target add x86_64-apple-darwin`
  = help: consider building the standard library from source with `cargo build -Zbuild-std`

// FIXME: I am lost here, is this the fee for funding spend or taker payment spend?
// if it's the former, where is the fee for the the taker payment spend?
let taker_payment_spend_trade_fee = match state_machine.taker_coin.get_receiver_trade_fee(stage).compat().await
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the fee paid by the receiver of a payment, it was an htlc spend fee in the legacy swap protocol. For v2, it should be 0 for maker like we agreed before (but maybe not in this PR, and we make it the taker payment spend fee for now), and for taker it should be the fee needed to spend the maker payment which is the same as the legacy protocol. Is this clear enough?
https://github.com/KomodoPlatform/komodo-defi-framework/blob/3c888aaf513eefe6ffbc99a6f2320dfe566223cb/mm2src/coins/lp_coins.rs#L3102-L3103

Copy link
Collaborator

Choose a reason for hiding this comment

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

and for taker it should be the fee needed to spend the maker payment which is the same as the legacy protocol.

As discussed in DM with @mariocynicys, this should be different than legacy protocol too since the redeem script is a bit different due to the immediate refund path. I will make this in progress again @mariocynicys

Actually should be called funding spend fee or taker payment fee, but we are just following the current code naming at the moment.

The negotiation goes as follows:
1- Taker and maker each decide how much this fee should be at the beginning of the swap.

2- The taker will send the maker their proposed fee during negotiation,
   if the maker deems the fee as low enough (less than 90% of the maker's own calculated fee),
   they will refuse to trade before the trade starts. Otherwise, they
   will continue and use this fee for validation later.

3- The maker will validate that the taker has accounted for this fee in
   the funding transaction.

4- The taker will validate that the taker payment preimage (generated by the maker)
   uses the negotiated fee or greater (greater number will deduct from the maker's trading volume anyway).
   If not, the taker will stop the trade since the maker is clearly stealing funds that isn't his
   which might affect the completion of the swap if the fee is low.
…o u64

The big decimal might be a fraction less than zero and gets converted to
zero when sent over the wire (because we send the fee as u64).

Scale it properly before sending it.
noticed this in the integration tests, the fee might be a fraction so
clamping it to one yields a fee higher than the actually one, thus fails
the ratio check
use a signature-size dummy date instead
taker payment spend is actually taker payment here, next commit should fix the naming inconsistencies
always rename discuss markers to fixmes since i forget about them xD
there is no calculatable tx_size for other tx (non-htlc) (like funding and maker payment txs). their fees depend really on the utxos we have and whether they are segwit or not.
used get_sender_trade_fee for now, though I think this method needs reviewing since its doc comment doesn't make a lot of sense
@mariocynicys mariocynicys requested a review from laruh October 14, 2024 13:18
@mariocynicys mariocynicys requested a review from shamardy October 26, 2024 16:21
@laruh
Copy link

laruh commented Nov 11, 2024

ps @mariocynicys this pr started to have conflicts

@shamardy
Copy link
Collaborator

Converted this to draft although we might need it for TPU

@shamardy shamardy marked this pull request as draft June 11, 2025 03:01
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.

4 participants