feat(tpu-v2): provide swap protocol versioning #2324

laruh · 2025-01-17T13:32:51Z

ordermatch structs which got swap_version field:

TakerOrderBuilder
MakerOrderBuilder
MakerOrder
MakerReserved
TakerRequest

swap structs. if swap started swap_version will be saved in db:

MySwapForRpc
TakerSwapStateMachine
MakerSwapStateMachine
TakerSwapDbRepr
MakerSwapDbRepr

swap version 2 is tpu v2 protocol
legacy swap protocol is 1 version

When Taker or Maker build order, the value const SWAP_VERSION: u32 = 2; is used for order Builder.
However in create_maker_order/lp_auto_buy if user didnt use ctx.use_trading_proto_v2() in config we set legacy protocol version to the order.

onur-ozkan

Is this blocking anything currently? These changes look like a hacky workaround rather than a being proper solution. It would be great if we can avoid this change.

onur-ozkan · 2025-01-20T11:00:54Z

mm2src/mm2_main/src/lp_ordermatch.rs

+fn legacy_swap_version() -> u32 { 1 }
+
+fn is_legacy_swap_version(swap_version: &u32) -> bool { *swap_version == 1 }


Suggested change

fn legacy_swap_version() -> u32 { 1 }

fn is_legacy_swap_version(swap_version: &u32) -> bool { *swap_version == 1 }

const fn legacy_swap_version() -> u32 { 1 }

fn is_legacy_swap_version(swap_version: &u32) -> bool { *swap_version == legacy_swap_version() }

Provided const fn 998e75a

laruh · 2025-01-20T11:27:37Z

Is this blocking anything currently? These changes look like a hacky workaround rather than a being proper solution. It would be great if we can avoid this change.

We need swap versioning to do a legacy fallback on order matching

dimxy · 2025-01-20T12:00:25Z

mm2src/mm2_main/src/lp_ordermatch.rs

+    /// In the future alls users will be using TPU V2 by default without "use_trading_proto_v2" configuration.
+    pub fn use_trading_proto_v2(mut self, use_trading_proto_v2: bool) -> Self {
+        if !use_trading_proto_v2 {
+            self.swap_version = 1;


Do we reserve only single value for legacy swaps?
I think legacy swaps may exist for sometime and we should allow version grow for them too.
I suggest use a structured encoding, for e.g.:
for legacy swaps: 1.x.y
for TPU: 2.x.y
(we could encode it into u32 as 1 * 10000 + x * 100 + y)

Explain how this is supposed to work and to be supported for kickstart process for example.

I really doubt we will use minor and patch versions in the current repo.
We can start legacy from 0 and reserve 1 version for legacy kmd burn implementation.
I think it will be the first and the last feature in legacy which will require explicit version separation.
zero fee shouldn't make many changes in utxo and evm code?

anyway everything which changes backward compatibility should update swap version "major" integer.

will legacy and legacy burn fee/zero fee be incompatible or not? That is only matters. Even if we dont reserve numbers before 2 for legacy, we could use next numbers after 2 for newer legacy versions. its just numbers.

Could you plz add some info how swap_version is used, to the PR description?
Was it meant to have just 1 and 2 values?

I am referring to the discussion in the team chat where it was discussed to get swap parties versions from the order matching. Maybe this PR should be reconsidered after we work out that concept.

I updated PR description

I am referring to the discussion in the team chat where it was discussed to get swap parties versions from the order matching.

This PR provides swap version from order matching

Maybe this PR should be reconsidered after we work out that concept.

This PR exists due to the reason that we still dont have any conclusion related to versioning. It is blocking TPU, to launch TPU in production, we need support for a legacy fallback.

I would tell we can change versioning later in other PRs, when we see by practice what versioning approach is preferable

dimxy · 2025-01-20T12:08:52Z

mm2src/mm2_main/src/lp_ordermatch.rs

+    /// In the future alls users will be using TPU V2 by default without "use_trading_proto_v2" configuration.
+    pub fn use_trading_proto_v2(mut self, use_trading_proto_v2: bool) -> Self {
+        if !use_trading_proto_v2 {
+            self.swap_version = 1;


we should use a const like LEGACY_SWAP_VERSION (indicating current version this code supports)

Current version this code/repo supports is this const SWAP_VERSION.
If use_trading_proto_v2:false it means user didnt enable TPU

reused const fn 53d4b40
we need const function for #[serde(default = "legacy_swap_version")], so no need to create one more const variable.

UPD: I refactored this function a bit according to Onur notes, so please check newer commits

onur-ozkan · 2025-01-21T07:44:19Z

mm2src/mm2_main/src/lp_ordermatch.rs

+    pub fn use_trading_proto_v2(mut self, use_trading_proto_v2: bool) -> Self {
+        if !use_trading_proto_v2 {
+            self.swap_version = legacy_swap_version();
+        }
+        self
+    }


Why not this?

Suggested change

pub fn use_trading_proto_v2(mut self, use_trading_proto_v2: bool) -> Self {

if !use_trading_proto_v2 {

self.swap_version = legacy_swap_version();

}

self

}

pub fn use_trading_proto_v2(mut self) -> Self {

self.swap_version = legacy_swap_version();

self

}

or, at least rename it to something like maybe_**

u suggested to change version to legacy protocol calling use_trading_proto_v2
please re read doc comment

/// When a new [TakerOrderBuilder::new] is created, it sets [SWAP_VERSION] by default. /// However, if user has not specified in the config to use TPU V2, /// the TakerOrderBuilder's swap_version is changed to legacy. /// In the future alls users will be using TPU V2 by default without "use_trading_proto_v2" configuration. pub fn use_trading_proto_v2(mut self, use_trading_proto_v2: bool) -> Self {

What I suggest is, either remove this boolean from the function and check it on the caller side or declare that in the function name with "maybe" prefix.

What I suggest is, either remove this boolean from the function and check it on the caller side

I dont get it, on the caller side it is where? Right now it looks like, you call use_trading_proto_v2 and set legacy

pub fn use_trading_proto_v2(mut self) -> Self { self.swap_version = legacy_swap_version(); self }

I updated function name e37c74e

I didn't pay attention to the logic, the usage was just unclear. Looking at the logic, it's even more unclear.. Function is called use_trading_proto_v2 but it downgrades version to legacy if given boolean is false.

Why not just this:

pub fn use_trading_proto_legacy(mut self) -> Self { self.swap_version = legacy_swap_version(); self }

and call it only when use_trading_proto_v2 is false?

the thing is that use_trading_proto_v2 initially is a config boolean flag, so use_trading_proto_legacy is supposed to be a question

ok I refactored function, now its purpose is only to set legacy protocol a62f56a

onur-ozkan · 2025-01-21T07:45:09Z

mm2src/mm2_main/src/lp_ordermatch.rs

+    pub fn use_trading_proto_v2(mut self, use_trading_proto_v2: bool) -> Self {
+        if !use_trading_proto_v2 {
+            self.swap_version = legacy_swap_version();
+        }
+        self


same as above

onur-ozkan · 2025-01-21T07:45:48Z

mm2src/mm2_main/src/lp_ordermatch.rs

+        let alice_swap_v = maker_match.request.swap_version;
+
+        // Start legacy swap if taker uses legacy protocol (version 1) or if conditions for trading_proto_v2 aren't met.
+        if alice_swap_v == 1u32 || !ctx.use_trading_proto_v2() {


Suggested change

if alice_swap_v == 1u32 || !ctx.use_trading_proto_v2() {

if alice_swap_v == legacy_swap_version() || !ctx.use_trading_proto_v2() {

Done d4c5a16

onur-ozkan · 2025-01-21T07:45:58Z

mm2src/mm2_main/src/lp_ordermatch.rs

+        let bob_swap_v = taker_match.reserved.swap_version;
+
+        // Start legacy swap if maker uses legacy protocol (version 1) or if conditions for trading_proto_v2 aren't met.
+        if bob_swap_v == 1u32 || !ctx.use_trading_proto_v2() {


Suggested change

if bob_swap_v == 1u32 || !ctx.use_trading_proto_v2() {

if bob_swap_v == legacy_swap_version() || !ctx.use_trading_proto_v2() {

Done d4c5a16

onur-ozkan · 2025-01-21T07:46:35Z

mm2src/mm2_main/src/lp_ordermatch/my_orders_storage.rs

@@ -706,6 +706,8 @@ mod tests {

    wasm_bindgen_test_configure!(run_in_browser);

+    const LEGACY_SWAP_V: u32 = 1;


legacy_swap_version can replace this need

Done d4c5a16

mm2src/mm2_main/src/ordermatch_tests.rs

mm2src/mm2_main/src/lp_ordermatch.rs

onur-ozkan · 2025-02-03T10:04:00Z

mm2src/mm2_main/src/lp_ordermatch/new_protocol.rs

+    #[serde(default = "legacy_swap_version")]
+    #[serde(skip_serializing_if = "is_legacy_swap_version")]
+    pub swap_version: u32,


It's worth to create a separate type (or even a separate module for handling everyting related to versioning for swaps) for swap versioning, e.g., something like:

struct SwapVersion { #[serde(default = "legacy_swap_version")] #[serde(skip_serializing_if = "is_legacy_swap_version")] swap_version: u32, }

which can help us to avoid this serialization rules applied all around the codebase.

I think this is a non-blocker improvement and I am okay to approve this PR without it. If you don't want to do it in this PR, can you leave a TODO note on one of them?

I like this idea!
it will be safer to work with version field wrapped into structure with all annotations which we need, then adding manually version with annotations to p2p messages structures.
Im going to do it in the current pr to not refactoring everything in next PR.

It's worth to create a separate type (or even a separate module for handling everyting related to versioning for swaps) for swap versioning, e.g., something like:

struct SwapVersion { #[serde(default = "legacy_swap_version")] #[serde(skip_serializing_if = "is_legacy_swap_version")] swap_version: u32, }

which can help us to avoid this serialization rules applied all around the codebase.

I tested such wrapper, deserialisation and serialisation dont work as we would like if you dont provide serde annotation above the pub swap_version: SwapVersion, field.
https://github.com/laruh/benchmarks_and_tests/blob/merge-candidate/serde_example/src/tests.rs

So we still have to add annotations, but I think that looks better:

And everything is in separate module

I will push changes for review

Here 6687599

mm2src/mm2_main/src/ordermatch_tests.rs

onur-ozkan

Thanks!

LGTM other than 2 minor notes which are not blockers.

onur-ozkan · 2025-02-05T06:20:59Z

mm2src/mm2_main/src/swap_versioning.rs

+impl SwapVersion {
+    pub(crate) fn is_legacy(&self) -> bool { self.version == legacy_swap_version() }


Suggested change

impl SwapVersion {

pub(crate) fn is_legacy(&self) -> bool { self.version == legacy_swap_version() }

impl SwapVersion {

pub(crate) const fn is_legacy(&self) -> bool { self.version == legacy_swap_version() }

Hmm why we can use const here if actual self.version value will be known at runtime?
I mean I can compile it, I thought it wont work. Is it bcz version has a primitive type?

version size is known at compile time.

onur-ozkan · 2025-02-05T06:22:00Z

mm2src/mm2_main/src/swap_versioning.rs

+
+#[derive(Copy, Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
+pub struct SwapVersion {
+    pub version: u32,


Not a blocker but u32 doesn't seem necessary for swap versioning, u8 should be more than enough.

I think it wont be a problem to move to u32, so we can start with u8
bab63f1

laruh · 2025-02-05T14:53:41Z

@onur-ozkan I noticed if user update the app having old swap entries in db, there could be error when we try to get such data, as structs expect version field
Added this commit de60aae

Well, changes are about TPU (not in production yet), but I think if migration 13 adds swap_version column, then it should also handle setting old swaps to version 1.

mariocynicys · 2025-02-06T09:29:04Z

@laruh you should add this in a new migration 14. Existing DBs that ran migration 13 will not run it again.
p.s. migrations should never be changed after written in general.

laruh · 2025-02-06T09:35:07Z

@laruh you should add this in a new migration 14. Existing DBs that ran migration 13 will not run it again. p.s. migrations should never be changed after written in general.

The current pr adds 13 migration. Dev doesn have it

komodo-defi-framework/mm2src/mm2_main/src/database.rs

Lines 122 to 137 in ff0eefc

    
           async fn statements_for_migration(ctx: &MmArc, current_migration: i64) -> Option<Vec<(&'static str, Vec<String>)>> { 
        
               match current_migration { 
        
                   1 => Some(migration_1(ctx).await), 
        
                   2 => Some(migration_2(ctx).await), 
        
                   3 => Some(migration_3()), 
        
                   4 => Some(migration_4()), 
        
                   5 => Some(migration_5()), 
        
                   6 => Some(migration_6()), 
        
                   7 => Some(migration_7()), 
        
                   8 => Some(migration_8()), 
        
                   9 => Some(migration_9()), 
        
                   10 => Some(migration_10(ctx).await), 
        
                   11 => Some(migration_11()), 
        
                   12 => Some(migration_12()), 
        
                   _ => None, 
        
               }

mariocynicys · 2025-02-06T09:36:40Z

The current pr adds 13 migration. Dev doesn have it

ah my bad. only checked the commit diff u linked :)

The base branch was changed.

updated my_swaps table migration, new swap_version field provided in Maker/TakerSwapStorage

laruh · 2025-02-08T15:22:54Z

Since the base branch was changed, I force-pushed an updated branch (cherry picked feature commits) with a clean git history to resolve conflicts

shamardy · 2025-02-08T16:24:06Z

Since the base branch was changed, I force-pushed an updated branch (cherry picked feature commits) with a clean git history to resolve conflicts

ooh, I deleted the base branch after merging to dev, sorry about that. No need for another review from @onur-ozkan as I will give it a review before merging.

laruh · 2025-02-08T16:38:31Z

ooh, I deleted the base branch after merging to dev, sorry about that. No need for another review from @onur-ozkan as I will give it a review before merging.

Its ok, I have old original branches locally and can push them if someone needs it

laruh added the status: pending review label Jan 17, 2025

laruh changed the title ~~feat(tpu-v2) provide swap protocol versioning field~~ feat(tpu-v2): provide swap protocol versioning Jan 17, 2025

onur-ozkan reviewed Jan 20, 2025

View reviewed changes

dimxy reviewed Jan 20, 2025

View reviewed changes

onur-ozkan reviewed Jan 21, 2025

View reviewed changes

mm2src/mm2_main/src/ordermatch_tests.rs Outdated Show resolved Hide resolved

laruh added the priority: high Important tasks that need attention soon. label Jan 23, 2025

laruh mentioned this pull request Jan 26, 2025

Trading protocol upgrade v2. #1895

Open

27 tasks

shamardy added the 2.4.0-beta label Feb 3, 2025

onur-ozkan reviewed Feb 3, 2025

View reviewed changes

shamardy requested a review from dimxy February 3, 2025 10:08

laruh mentioned this pull request Feb 3, 2025

Protocol versioning support #2337

Open

onur-ozkan previously approved these changes Feb 5, 2025

View reviewed changes

laruh mentioned this pull request Feb 7, 2025

fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret #2261

Merged

Base automatically changed from fix-tpu-v2-wait-for-payment-spend to dev February 8, 2025 14:04

laruh added 4 commits February 8, 2025 21:50

swap_version field

65a8df9

add swap_version field in MySwapForRpc structure,

7e4ca64

updated my_swaps table migration, new swap_version field provided in Maker/TakerSwapStorage

review: use const fn

d768995

review: call const legacy func in use_trading_proto_v2 fnc

41c7037

laruh added 9 commits February 8, 2025 21:50

review: use const fn in more places

53a1200

review: rename function

8bf8cbb

review: provide set_legacy_swap_v func

5e56b2f

review: compare references

d19867c

review: remove const in tests

e3c9b26

review: create swap_versioning module

306e15b

review: make is_legacy func const

5a3dfed

review: make SwapVersion::version u8

0b8deab

keep backward compatibility for old swaps in database

fe1bfc3

laruh force-pushed the fix-tpu-v2-wait-for-payment-spend-swap-version branch from de60aae to fe1bfc3 Compare February 8, 2025 14:56

laruh requested a review from onur-ozkan February 8, 2025 15:23

laruh removed the request for review from onur-ozkan February 8, 2025 16:39

		fn legacy_swap_version() -> u32 { 1 }

		fn is_legacy_swap_version(swap_version: &u32) -> bool { *swap_version == 1 }

	if alice_swap_v == 1u32 \|\| !ctx.use_trading_proto_v2() {
	if alice_swap_v == legacy_swap_version() \|\| !ctx.use_trading_proto_v2() {

	if bob_swap_v == 1u32 \|\| !ctx.use_trading_proto_v2() {
	if bob_swap_v == legacy_swap_version() \|\| !ctx.use_trading_proto_v2() {

		@@ -706,6 +706,8 @@ mod tests {

		wasm_bindgen_test_configure!(run_in_browser);

		const LEGACY_SWAP_V: u32 = 1;

		impl SwapVersion {
		pub(crate) fn is_legacy(&self) -> bool { self.version == legacy_swap_version() }

feat(tpu-v2): provide swap protocol versioning #2324

Are you sure you want to change the base?

feat(tpu-v2): provide swap protocol versioning #2324

Conversation

laruh commented Jan 17, 2025 • edited Loading

onur-ozkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh commented Feb 5, 2025

mariocynicys commented Feb 6, 2025 • edited Loading

laruh commented Feb 6, 2025

mariocynicys commented Feb 6, 2025

laruh commented Feb 8, 2025

shamardy commented Feb 8, 2025

laruh commented Feb 8, 2025

laruh commented Jan 17, 2025 •

edited

Loading

laruh Jan 21, 2025 •

edited

Loading

laruh Jan 21, 2025 •

edited

Loading

onur-ozkan Jan 21, 2025 •

edited

Loading

mariocynicys commented Feb 6, 2025 •

edited

Loading