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

feat(trading-proto-upgrade): sql storage wip + protocol enhancement #1980

Merged
merged 35 commits into from
Oct 30, 2023

Conversation

artemii235
Copy link
Member

@artemii235 artemii235 commented Oct 2, 2023

#1895

What's done:

  • Removed access by index feat(trading-proto-upgrade): UTXO PoC + State machine refactor #1927 (comment)
  • Changed OP_CHECKMULTISIG to <pubkey0> OP_CHECKSIGVERIFY <pubkey1> OP_CHECKSIG chore(release): v1.0.7-beta  #1936 (comment)
  • Implemented storing upgraded swaps data to SQLite DB (still WIP)
  • Implemented protocol enhancement for UTXO coins by adding one more funding tx for taker, which can be reclaimed immediately if maker back-outs.
  • Done some refactoring, added CoinAssocTypes trait representing coin associated types. It makes implementation more generic, helps to avoid repeated transactions and other data deserialization in swaps operations, etc.

Notes

  • Implementation still covers only success swap case.

@artemii235 artemii235 added the in progress Changes will be made from the author label Oct 2, 2023
@artemii235 artemii235 added under review and removed in progress Changes will be made from the author labels Oct 2, 2023
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! First review iteration!

mm2src/coins/lp_coins.rs Show resolved Hide resolved
mm2src/coins/utxo.rs Outdated Show resolved Hide resolved
Comment on lines +1343 to +1356
let expected_fee = coin
.get_htlc_spend_fee(DEFAULT_SWAP_TX_SPEND_SIZE, &FeeApproxStage::WithoutApprox)
.await?;

let actual_fee = funding_amount - payment_amount;

let fee_div = expected_fee as f64 / actual_fee as f64;

if !(0.9..=1.1).contains(&fee_div) {
return MmError::err(ValidateTakerFundingSpendPreimageError::UnexpectedPreimageFee(format!(
"Too large difference between expected {} and actual {} fees",
expected_fee, actual_fee
)));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid such issues with validation, I propose that the funding spending fee should be predetermined by the taker and added to the total_amount here

let total_amount = &args.dex_fee_amount + &args.premium_amount + &args.trading_amount;
The taker can then validate whether the fee is greater than or equal to the fee they initially decided upon.

It's a question of who should pay the funding spending fee actually, right now the maker pays for it. If the taker sets the fee beforehand, and the fee subsequently rises when the maker is signing the funding spending transaction, the maker could pay the additional amount to ensure that the transaction is processed. However, I think it's better for the maker to just use the exact fee the taker decided to avoid the taker lowballing them. The maker should never sign a transaction with a fee below what the taker decided though, they should use what the taker decided if the fee has decreased since the taker decided upon it.

P.S. you can work on this on next sprints, I think we need to take more time to think about the best way to handle this.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a question of who should pay the funding spending fee actually, right now the maker pays for it.

Yeah, it's a tricky part of funding tx approach 🙂 I added it to the issue checklist and will add to the next sprint discussion points.

Artem Vitae added 3 commits October 4, 2023 10:02
…ration-3

# Conflicts:
#	mm2src/mm2_main/tests/docker_tests/swap_watcher_tests.rs
@artemii235 artemii235 mentioned this pull request Oct 2, 2023
24 tasks
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Next review iteration! Only 2 comments!
Will do one last review iteration after this where I check the maker_swap_v2 and taker_swap_v2 code one more time :)

@@ -998,6 +1010,35 @@ impl From<SavedSwap> for MySwapStatusResponse {
}

/// Returns the status of swap performed on `my` node
#[cfg(not(target_arch = "wasm32"))]
pub async fn my_swap_status(ctx: MmArc, req: Json) -> Result<Response<Vec<u8>>, String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it's time to implement my_swap_status v2 using "mmrpc": "2.0" similar to withdraw and other methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm thinking about this too, good point to discuss in the next sprint 🙂

mm2src/mm2_main/src/lp_swap/maker_swap_v2.rs Outdated Show resolved Hide resolved
shamardy
shamardy previously approved these changes Oct 5, 2023
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Great work as always!
I realised that we are not settled yet on swap payments confirmation requirements so I will do the whole check of swaps code once we settle on them and they are implemented.

@@ -483,76 +865,125 @@ impl<MakerCoin: MmCoin, TakerCoin: MmCoin + SwapOpsV2> State for TakerPaymentSen

if let Err(e) = state_machine.maker_coin.wait_for_confirmations(input).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.

I think taker will have to wait for confirmation of maker payment before spending the funding transaction. Concurrent confirmations of both taker and maker payments were only viable before introducing immediate refund path for taker. I think you are aware of this but didn't get to change it yet :)
P.S. This is just a reminder for next sprints where confirmations need to be rechecked and probably changed, ref. next comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed that we can still keep concurrent confirmations, though it should be carefully reviewed with regard to possible maker payment cancellation/replacement with another tx. Added to the checklist and will add to the next sprint discussion points.

let input = ConfirmPaymentInput {
payment_tx: self.taker_payment.tx_hex.0.clone(),
payment_tx: self.taker_funding.tx_hex(),
Copy link
Collaborator

@shamardy shamardy Oct 5, 2023

Choose a reason for hiding this comment

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

Next state is TakerPaymentConfirmed, shouldn't this be the taker_payment not taker_funding. We will also require a confirmation for the taker_funding in a previous step/state. This is also a comment for next sprints.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it should be taker payment, fixed.

shamardy
shamardy previously approved these changes Oct 9, 2023
@shamardy
Copy link
Collaborator

@artemii235 please merge with latest dev and then merge this PR to dev :)

…ration-3

# Conflicts:
#	mm2src/coins/test_coin.rs
#	mm2src/coins/utxo/utxo_common.rs
#	mm2src/coins/utxo/utxo_standard.rs
#	mm2src/mm2_main/src/lp_ordermatch.rs
#	mm2src/mm2_main/src/lp_swap.rs
Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

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

I have a question.
Why did you decide to use same struct names for stored negotiation data in maker and taker swaps?
I think its preferable to use different names like StoredTaker/MakerNegotiationData, especially when both structs are public. Or may be I didnt get your idea.

use std::marker::PhantomData;
use uuid::Uuid;

// This is needed to have Debug on messages
#[allow(unused_imports)] use prost::Message;

/// Negotiation data representation to be stored in DB.
#[derive(Debug, Deserialize, Serialize)]
pub struct StoredNegotiationData {
Copy link
Member

Choose a reason for hiding this comment

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

Here

Copy link
Member Author

Choose a reason for hiding this comment

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

These structs have fields with different semantics/names. Possibly, they will differ even more in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I mean, if you import structs in other place, it will be comfortable to use them with different names. I dont tell that they are the same objects, of course not :)

Copy link
Member

Choose a reason for hiding this comment

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

I dont insist, just was curious and wanted to note it for the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

it will be comfortable to use them with different names

I agree, but I had to make them and Maker/TakerSwapEvent public since they are used in the public interface here:

I will look for a way to remove pub in next iterations, added to the issue checklist, thanks!

use std::marker::PhantomData;
use uuid::Uuid;

// This is needed to have Debug on messages
#[allow(unused_imports)] use prost::Message;

/// Negotiation data representation to be stored in DB.
#[derive(Debug, Deserialize, Serialize)]
pub struct StoredNegotiationData {
Copy link
Member

Choose a reason for hiding this comment

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

and here

Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

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

Great work! LGTM!

ps: thanks for doc coms :)

@artemii235 artemii235 merged commit 8d0a583 into dev Oct 30, 2023
28 of 30 checks passed
@artemii235 artemii235 deleted the swap-proto-upgrade-iteration-3 branch October 30, 2023 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants