Provided const fn 998e75a

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

Why not this?

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

same as above

Done d4c5a16

Done d4c5a16

legacy_swap_version can replace this need

Done d4c5a16

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

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.

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

About naming vars:
I think SET_DEFAULT_SWAP_VERSION here actually means not 'default' but 'existing'.

Plus, there is also a comment in the code:

/// When a new [TakerOrderBuilder::new] is created, it sets [SWAP_VERSION] by default.

So I guess, SWAP_VERSION is in fact SWAP_VERSION_DEFAULT

Thanks for the catch, fixed here 731e605

I think it would be useful to add the version to stats_swaps table as well. It should be done in another PR though as this is should be merged soon.

I just noticed that currently we don't need the swap version in my_swaps table as we have swap_type https://github.com/KomodoPlatform/komodo-defi-framework/blob/731e605fc2b53b9f4afa9e13e3682c5be7ed9cf6/mm2src/mm2_main/src/database/my_swaps.rs#L89 https://github.com/KomodoPlatform/komodo-defi-framework/blob/39515a9f3ea1089bb462e99c8cafb1049a920dbd/mm2src/mm2_main/src/lp_swap.rs#L152-L154 but version can be different from type in the future, for instance MAKER_SWAP_V2_TYPE which s TPU Maker can have multiple versions but one type. Just thought to mention this and the redundancy we have.

I just noticed that currently we don't need the swap version in my_swaps table as we have swap_type

From what I see these TYPE consts were provided to separate legacy and swap_v2 taker/maker data formats in database. I think it is something different from swap protocol version.

Well may be in practice we will see, will taker/maker types be different from swap protocol version or not.

ps: This field is useful for MySwapForRpc, as it can be retrieved from both get_taker_swap_data_for_rpc and get_maker_swap_data_for_rpc. Having a ready swap_version field simplifies the process of determining the swap version.

I understand, that's why I said currently :)
What I meant was that we could have converted this type to the version without adding a new column for now, that's all. But I like having version explicitly and resolving this comment.

Leaving it only for stats_swaps #2324 (comment)