[r2r] Include maker/taker pubkeys in MM2.db stats_swaps table by borngraced · Pull Request #1665 · GLEECBTC/komodo-defi-framework

borngraced · 2023-02-19T22:15:22Z

resolves: #1646

Signed-off-by: borngraced <samiodev@icloud.com>

sergeyboyko0791

Thank you for the great enhancement! I have one important question

sergeyboyko0791 · 2023-02-20T15:10:35Z

+    taker_coin_swap_contract: Vec<u8>,
+    maker_coin_htlc_pub: Vec<u8>,
+    taker_coin_htlc_pub: Vec<u8>,
+    persistent_pubkey: Vec<u8>,


Please try to avoid adding NegotiationDataV4 message. I even think this new variant might break backwards compatibility.
I personally think it's better to store maker_coin_htlc_pub as maker_pubkey and taker_coin_htlc_pub as taker_pubkey. @shamardy what do you think?

waiting for @shamardy feedback on this...

I agree with @sergeyboyko0791 here. Using persistent_pubkey also leaks private information for private ARRR swaps.

@borngraced is this PR ready for review? NegotiationDataV4 is still not removed from the code.

Oh sorry, I missed that! and yes it's ready for review.

onur-ozkan

lgtm, 1 question

onur-ozkan · 2023-02-21T08:38:37Z

+    "ALTER TABLE stats_swaps ADD COLUMN maker_pubkey VARCHAR(255);",
+    "ALTER TABLE stats_swaps ADD COLUMN taker_pubkey VARCHAR(255);",


Can't just "ALTER TABLE stats_swaps ADD COLUMN maker_pubkey VARCHAR(255), ADD COLUMN taker_pubkey VARCHAR(255); work?

yes thanks.. it could be this also, but I followed a convention that I already used before https://github.com/KomodoPlatform/atomicDEX-API/blob/54081307cd093cce321939b618a684ad16c915a7/mm2src/mm2_main/src/database/stats_swaps.rs#L49-L52

Signed-off-by: borngraced <samiodev@icloud.com>

shamardy

Good start :)
First review iteration, we just need to be all inline about implementation details at this point.

shamardy · 2023-02-28T11:30:05Z

    "Negotiated",
    "TakerFeeSent",
-    "TakerPaymentInstructionsReceived",
+    "TakerPaymentInstructionscReceived",


You seem to have changed this event unintentionally

shamardy · 2023-02-28T14:04:31Z

 #[derive(Debug, Default, PartialEq, Serialize, Deserialize)]
 pub struct MakerSavedSwap {
    pub uuid: Uuid,
+    pub my_persistence_pub: Option<H264Json>,


Can you please explain why you added my_persistence_pub to MakerSavedSwap/TakerSavedSwap. Isn't it available in TakerSwapData/MakerSwapDatahttps://github.com/KomodoPlatform/atomicDEX-API/blob/f39c52dff9bdad9b1b600722b88c8823f8bd0147/mm2src/mm2_main/src/lp_swap/taker_swap.rs#L513 https://github.com/KomodoPlatform/atomicDEX-API/blob/f39c52dff9bdad9b1b600722b88c8823f8bd0147/mm2src/mm2_main/src/lp_swap/maker_swap.rs#L143 I really just think we should add maker_coin_htlc_pubkeyand taker_coin_htlc_pubkey to the DB as is, there should be 4 pubkeys in this case (maker_pubkey_for_maker_coin, maker_pubkey_for_taker_coin, taker_pubkey_for_maker_coin, taker_pubkey_for_taker_coin), please note that these 4 can be different when using HD wallet functionality, we can get @sergeyboyko0791 input on this.

@cipig which exact pubkey do you parse from the json files? I can see that in stats_swaps in seednodes there will be either a MakerSavedSwap or TakerSavedSwap so I guess you use my_persistence_pub and one of maker_coin_htlc_pubkey/taker_coin_htlc_pubkey from MakerNegotiationData/TakerNegotiationData

@shamardy, with the help of Sergey's input, I got this context

There can be multiple cases. Let's consider the Maker side only:

Maker sends NegotiationDataV1 or NegotiationDataV2 where NegotiationDataV1/2::persistent_pubkey is considered as maker_pubkey (its persistent pubkey that identifiers him uniquely)

Maker sends NegotiationDataV3 where NegotiationDataV3::maker_coin_htlc_pub and NegotiationDataV3::taker_coin_htlc_pub are the same. If the maker_coin and taker_coin tickers are not ARRR (or other private coin), we can consider NegotiationDataV3::m/taker_coin_htlc_pub as maker_pubkey (its persistent pubkey that identifiers him uniquely)

Maker sends NegotiationDataV3 where NegotiationDataV3::maker_coin_htlc_pub and NegotiationDataV3::taker_coin_htlc_pub are different. In that case we probably should try to figure out what coin is private in the swap, and what coin is not. The private coin pubkey should have a random m/taker_coin_htlc_pub that is unique between all swaps. But the public coin should have the same public key as the persistent_pubkey

and also the data in

pub my_persistence_pub: Option<H264Json>,

Is the persistence_pub from T/MakerSwap
https://github.com/KomodoPlatform/atomicDEX-API/blob/a6f9a0ea383d6f87f2157bb6598a76eb7fa10071/mm2src/mm2_main/src/lp_swap/taker_swap.rs#L464

As we discussed DM, there can be the case when it's better not to share the persistent public key. For example, when you exchange two private coins (like ARRR).
But on the other hand, I am not sure if maker_coin_maker_htlc_pubkey, maker_coin_taker_htlc_pubkey, taker_coin_maker_htlc_pubkey, taker_coin_taker_htlc_pubkey pubkeys can be useful as they can be different, and we can't recognise a node that performed the swaps.

Is the persistence_pub from T/MakerSwap

@borngraced what I meant is that you can use the one from these events https://github.com/KomodoPlatform/atomicDEX-API/blob/f39c52dff9bdad9b1b600722b88c8823f8bd0147/mm2src/mm2_main/src/lp_swap/maker_swap.rs#L1538
https://github.com/KomodoPlatform/atomicDEX-API/blob/f39c52dff9bdad9b1b600722b88c8823f8bd0147/mm2src/mm2_main/src/lp_swap/taker_swap.rs#L635 instead of adding it to MakerSavedSwap/TakerSavedSwap

As we discussed DM, there can be the case when it's better not to share the persistent public key. For example, when you exchange two private coins (like ARRR).
But on the other hand, I am not sure if maker_coin_maker_htlc_pubkey, maker_coin_taker_htlc_pubkey, taker_coin_maker_htlc_pubkey, taker_coin_taker_htlc_pubkey pubkeys can be useful as they can be different, and we can't recognise a node that performed the swaps.

@smk762 @ca333 We need your inputs on this, it all comes down to if we want to track one pubkey for all swap operations related to a node, or track the pubkeys related to the used HD account for the swap. This can be considered another issue different from the current one. As for the current one I think we should just use the pubkeys @cipig used to parse from the json files.

Perhaps we can use the PeerID for private coins?
The reason for the request was to gather stats on most active users (and able to delineate maker/taker activity), so for HD perhaps a single identifier for the node would be best.

Signed-off-by: borngraced <samiodev@icloud.com>

shamardy

Next iteration! We need a unit test added to make sure that the right pubkeys are saved.

shamardy · 2023-03-06T18:00:51Z

+                swap_pubkeys.maker = Some(started.my_persistent_pub);
+                swap_pubkeys.taker = started.maker_coin_htlc_pubkey;


Aren't those 2 pubkeys the maker's? can you please add a unit test to test that the right pubkeys are saved to DB? you should use 3 nodes for the test (taker/maker/seednode) and use stats_swap_status RPC to check that the right pubkeys are saved to the seednode's DB.

shamardy · 2023-03-06T18:01:39Z

+                swap_pubkeys.maker = started.maker_coin_htlc_pubkey;
+                swap_pubkeys.taker = Some(started.my_persistent_pub);


Same for those 2 pubkeys, aren't they both the taker's?

@shamardy this is ready for another round of review!

Signed-off-by: borngraced <samuelonoja970@gmail.com>

# Conflicts: # mm2src/mm2_main/src/lp_swap/maker_swap.rs # mm2src/mm2_main/src/lp_swap/taker_swap.rs

shamardy

Next iteration! Probably the last one :)

Signed-off-by: borngraced <samuelonoja970@gmail.com>

shamardy

Final review :)
Just some minor stuff left, otherwise everything else looks good.

Signed-off-by: borngraced <samuelonoja970@gmail.com>

shamardy

Thank you for your patience on this PR. LGTM!

* add change logs for PRs merged to dev only * remove {version} - {date} and add dev branch instead * add ibc transfer change logs * add lightning PR #1655 to change logs * add changelog for #1706 * add changelog for #1694, #1665 --------- Reviewed-by: laruh, caglaryucekaya <caglaryucekaya@gmail.com>

borngraced added 2 commits February 18, 2023 00:23

saved dev state

430b2a9

Signed-off-by: borngraced <samiodev@icloud.com>

include pubkeys in stats_swaps.rs

4bf3833

Signed-off-by: borngraced <samiodev@icloud.com>

borngraced requested review from caglaryucekaya, onur-ozkan and shamardy February 19, 2023 22:15

borngraced self-assigned this Feb 19, 2023

borngraced added 4 commits February 19, 2023 23:16

remove println

630a9fe

Signed-off-by: borngraced <samiodev@icloud.com>

Merge branch 'dev' into add-pubkeys-to-stats-swap

b9a233e

refactor get_swap_pubkeys -> swap_pubkeys

c860d15

Signed-off-by: borngraced <samiodev@icloud.com>

change ADD_MAKER_TAKER_PUBKEYS types

12b2b41

Signed-off-by: borngraced <samiodev@icloud.com>

sergeyboyko0791 suggested changes Feb 20, 2023

View reviewed changes

borngraced changed the title ~~[wip] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [r2r] Include maker/taker pubkeys in MM2.db stats_swaps table Feb 20, 2023

borngraced mentioned this pull request Feb 20, 2023

[release] 1.0.0-beta #1576

Merged

onur-ozkan previously approved these changes Feb 21, 2023

View reviewed changes

remove NegotiationDataV4

0c642a2

Signed-off-by: borngraced <samiodev@icloud.com>

borngraced dismissed onur-ozkan’s stale review via 0c642a2 February 24, 2023 01:26

Merge branch 'dev' into add-pubkeys-to-stats-swap

6b057da

borngraced changed the title ~~[r2r] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [wip] Include maker/taker pubkeys in MM2.db stats_swaps table Feb 24, 2023

minor changes — ready for review

dc612e9

Signed-off-by: borngraced <samiodev@icloud.com>

borngraced changed the title ~~[wip] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [r2r] Include maker/taker pubkeys in MM2.db stats_swaps table Feb 25, 2023

borngraced added 2 commits February 26, 2023 23:50

remove is_private_coin fn

f39c52d

Signed-off-by: borngraced <samiodev@icloud.com>

remove NegotiationDataV4

76d0929

Signed-off-by: borngraced <samiodev@icloud.com>

shamardy requested changes Feb 28, 2023

View reviewed changes

remove my_persistent_pub from maker/taker_swap

4aa8539

Signed-off-by: borngraced <samiodev@icloud.com>

borngraced changed the title ~~[r2r] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [wip] Include maker/taker pubkeys in MM2.db stats_swaps table Mar 1, 2023

borngraced added 2 commits March 3, 2023 17:07

remove my_persistent_pub from maker/taker_swap

ef088e3

Signed-off-by: borngraced <samiodev@icloud.com>

fix unit tests

a581dfb

Signed-off-by: borngraced <samiodev@icloud.com>

borngraced changed the title ~~[wip] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [r2r] Include maker/taker pubkeys in MM2.db stats_swaps table Mar 3, 2023

shamardy requested changes Mar 6, 2023

View reviewed changes

borngraced changed the title ~~[r2r] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [wip] Include maker/taker pubkeys in MM2.db stats_swaps table Mar 7, 2023

save dev state — stats_swap swap coin pubkeys unit test

fe856bf

Signed-off-by: borngraced <samuelonoja970@gmail.com>

borngraced changed the title ~~[wip] Include maker/taker pubkeys in MM2.db stats_swaps table~~ [r2r] Include maker/taker pubkeys in MM2.db stats_swaps table Mar 8, 2023

borngraced added 4 commits March 8, 2023 02:25

finish impl unit test

275bf27

Signed-off-by: borngraced <samuelonoja970@gmail.com>

remove #[ignore]

012c2a3

Signed-off-by: borngraced <samuelonoja970@gmail.com>

Merge remote-tracking branch 'origin/dev' into add-pubkeys-to-stats-swap

526ffa0

# Conflicts: # mm2src/mm2_main/src/lp_swap/maker_swap.rs # mm2src/mm2_main/src/lp_swap/taker_swap.rs

Merge remote-tracking branch 'origin/dev' into add-pubkeys-to-stats-swap

12ec822

shamardy requested changes Mar 10, 2023

View reviewed changes

Comment thread mm2src/mm2_main/src/lp_swap/maker_swap.rs Outdated

Comment thread mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs Outdated

borngraced and others added 2 commits March 12, 2023 06:02

minor changes

5f5a3ea

Signed-off-by: borngraced <samuelonoja970@gmail.com>

remove unused

f40f582

Signed-off-by: borngraced <samuelonoja970@gmail.com>

shamardy reviewed Mar 13, 2023

View reviewed changes

Comment thread mm2src/mm2_main/src/lp_swap.rs Outdated

Comment thread mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs Outdated

undo unit tests changes

0e3f647

Signed-off-by: borngraced <samuelonoja970@gmail.com>

shamardy approved these changes Mar 14, 2023

View reviewed changes

shamardy merged commit 5faec02 into dev Mar 14, 2023

shamardy deleted the add-pubkeys-to-stats-swap branch March 14, 2023 12:31

shamardy added a commit that referenced this pull request Mar 14, 2023

add changelog for #1694, #1665

dd3a42d

ca333 mentioned this pull request Mar 27, 2023

[release] 1.0.1-beta #1721

Merged

		"ALTER TABLE stats_swaps ADD COLUMN maker_pubkey VARCHAR(255);",
		"ALTER TABLE stats_swaps ADD COLUMN taker_pubkey VARCHAR(255);",

		swap_pubkeys.maker = Some(started.my_persistent_pub);
		swap_pubkeys.taker = started.maker_coin_htlc_pubkey;

		swap_pubkeys.maker = started.maker_coin_htlc_pubkey;
		swap_pubkeys.taker = Some(started.my_persistent_pub);

Conversation

borngraced commented Feb 19, 2023

Uh oh!

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borngraced Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

borngraced Feb 28, 2023 •

edited

Loading