Skip to content

fix(swaps): maintain legacy compatibility for negotiation messages#2353

Merged
shamardy merged 4 commits intodevfrom
fix-neg-bc
Feb 12, 2025
Merged

fix(swaps): maintain legacy compatibility for negotiation messages#2353
shamardy merged 4 commits intodevfrom
fix-neg-bc

Conversation

@shamardy
Copy link
Copy Markdown
Collaborator

@shamardy shamardy commented Feb 11, 2025

@shamardy shamardy mentioned this pull request Feb 11, 2025
Merged
shamardy
shamardy commented Feb 11, 2025
View reviewed changes
mm2src/mm2_main/src/lp_swap.rs Outdated
Comment on lines +859 to +860
#[serde(serialize_with = "H264::serialize_as_byte_seq")]
#[serde(deserialize_with = "H264::deserialize_as_bytes")]
Copy link
Copy Markdown
Collaborator Author

@shamardy shamardy Feb 11, 2025

Choose a reason for hiding this comment

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

decided to use new ser/der functions instead of modifying the old ones as the old ones needed for backward compatibility for swap files ref https://github.com/KomodoPlatform/komodo-defi-framework/blob/39515a9f3ea1089bb462e99c8cafb1049a920dbd/mm2src/mm2_main/src/lp_swap/taker_swap.rs#L660

@shamardy shamardy mentioned this pull request Feb 11, 2025
Merged
borngraced
borngraced suggested changes Feb 11, 2025
View reviewed changes
Copy link
Copy Markdown

@borngraced borngraced left a comment

Choose a reason for hiding this comment

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

minor notes

onur-ozkan
onur-ozkan previously approved these changes Feb 12, 2025
View reviewed changes
Copy link
Copy Markdown

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Lgtm

@cipig
Copy link
Copy Markdown

cipig commented Feb 12, 2025 •
edited
Loading

updated a maker to this and started a swap with it from an older taker
swap worked fine on both, but seeing this error on taker:

12 14:57:06, mm2_main::lp_swap::taker_swap:1884] INFO Maker payment spend tx cdfcd2d44cb212605b47a442447b7c96a35b6f84263dfccf48e11b13f8b8abc8
12 14:57:44, mm2_main::lp_network:169] ERROR lp_swap:350] Couldn't deserialize swap msg to either 'SwapMsg': invalid type: integer `123`, expected struct SignedMessageSerdeHelper or to 'SwapStatus': missing field `taker` at line 1 column 6481
12 14:57:52, mm2_main::lp_swap::taker_swap:1916] INFO Maker payment spend confirmed

where does it come from? can we ignore it?

@shamardy
Copy link
Copy Markdown
Collaborator Author

shamardy commented Feb 12, 2025

updated a maker to this and started a swap with it from an older taker swap worked fine on both, but seeing this error on taker:

12 14:57:06, mm2_main::lp_swap::taker_swap:1884] INFO Maker payment spend tx cdfcd2d44cb212605b47a442447b7c96a35b6f84263dfccf48e11b13f8b8abc8
12 14:57:44, mm2_main::lp_network:169] ERROR lp_swap:350] Couldn't deserialize swap msg to either 'SwapMsg': invalid type: integer `123`, expected struct SignedMessageSerdeHelper or to 'SwapStatus': missing field `taker` at line 1 column 6481
12 14:57:52, mm2_main::lp_swap::taker_swap:1916] INFO Maker payment spend confirmed

where does it come from? can we ignore it?

It's due to the below https://github.com/KomodoPlatform/komodo-defi-framework/blob/39515a9f3ea1089bb462e99c8cafb1049a920dbd/mm2src/mm2_main/src/lp_swap/maker_swap.rs#L147-L148
It seems we should have used serde rename @laruh for p2p backward compatibility when serializing and sending swap status to an old node. Will fix it in this PR.

@shamardy
use serde rename instead of alias
cdd7c61 
This is for `TakerSwapData::maker_pubkey` and `MakerSwapData::taker_pubkey` to fix p2p backward compatibility when serializing to old nodes
@shamardy
Copy link
Copy Markdown
Collaborator Author

shamardy commented Feb 12, 2025

@cipig can you please check if the below error doesn't occur anymore

12 14:57:44, mm2_main::lp_network:169] ERROR lp_swap:350] Couldn't deserialize swap msg to either 'SwapMsg': invalid type: integer `123`, expected struct SignedMessageSerdeHelper or to 'SwapStatus': missing field `taker` at line 1 column 6481

cipig
cipig approved these changes Feb 12, 2025
View reviewed changes
Copy link
Copy Markdown

@cipig cipig left a comment

Choose a reason for hiding this comment

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

error is now gone, swaps still working fine

@shamardy shamardy merged commit 51454d1 into dev Feb 12, 2025
@shamardy shamardy deleted the fix-neg-bc branch February 12, 2025 22:47
dimxy pushed a commit that referenced this pull request Feb 16, 2025
Merge branch 'dev' into lr-swap-wip
a52ebc0 
* dev:
  fix(derive_key_from_path): check length of current_key_material (#2356)
  chore(release): bump mm2 version to 2.4.0-beta (#2346)
  fix(tests): add additional testnet sepolia nodes to test code (#2358)
  fix(swaps): maintain legacy compatibility for negotiation messages (#2353)
  refactor(SwapOps): impl defaults for protocol specific swapops fns (#2354)
  feat(tpu-v2): provide swap protocol versioning (#2324)
  feat(wallet): add change mnemonic password rpc (#2317)
  fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret (#2261)
  feat(tendermint): unstaking/undelegation (#2330)
  fix(utxo-withdraw): get hw ctx only when `PrivKeyPolicy` is trezor (#2333)
  feat(event-streaming): API-driven subscription management (#2172)
  fix(hash-types): remove panic, enforce fixed-size arrays (#2279)
  fix(ARRR): store unconfirmed change output (#2276)
  feat(tendermint): staking/delegation (#2322)
  chore(deps): `timed-map` migration (#2247)
  fix(mem-leak): `running_swap` never shrinks (#2301)
  chore(dep-bump): libp2p (#2326)
  refactor(build script): rewrite the main build script (#2319)
dimxy pushed a commit that referenced this pull request Feb 16, 2025
Merge branch 'dev' into lr-swap-wip
b86f433 
* dev:
  fix(derive_key_from_path): check length of current_key_material (#2356)
  chore(release): bump mm2 version to 2.4.0-beta (#2346)
  fix(tests): add additional testnet sepolia nodes to test code (#2358)
  fix(swaps): maintain legacy compatibility for negotiation messages (#2353)
  refactor(SwapOps): impl defaults for protocol specific swapops fns (#2354)
  feat(tpu-v2): provide swap protocol versioning (#2324)
  feat(wallet): add change mnemonic password rpc (#2317)
  fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret (#2261)
  feat(tendermint): unstaking/undelegation (#2330)
  fix(utxo-withdraw): get hw ctx only when `PrivKeyPolicy` is trezor (#2333)
  feat(event-streaming): API-driven subscription management (#2172)
  fix(hash-types): remove panic, enforce fixed-size arrays (#2279)
  fix(ARRR): store unconfirmed change output (#2276)
  feat(tendermint): staking/delegation (#2322)
  chore(deps): `timed-map` migration (#2247)
  fix(mem-leak): `running_swap` never shrinks (#2301)
  chore(dep-bump): libp2p (#2326)
  refactor(build script): rewrite the main build script (#2319)
dimxy pushed a commit to dimxy/komodo-defi-framework that referenced this pull request Feb 24, 2025
Merge branch 'dev' into fix-utxo-tx-fee-for-change
534006e 
* dev: (24 commits)
  fix(eth-tpu): remove state from funding validation (GLEECBTC#2334)
  improvement(rpc-server): rpc server dynamic port allocation (GLEECBTC#2342)
  fix(tests): fix or ignore unstable tests (GLEECBTC#2365)
  fix(fs): make `filter_files_by_extension` return only files (GLEECBTC#2364)
  fix(derive_key_from_path): check length of current_key_material (GLEECBTC#2356)
  chore(release): bump mm2 version to 2.4.0-beta (GLEECBTC#2346)
  fix(tests): add additional testnet sepolia nodes to test code (GLEECBTC#2358)
  fix(swaps): maintain legacy compatibility for negotiation messages (GLEECBTC#2353)
  refactor(SwapOps): impl defaults for protocol specific swapops fns (GLEECBTC#2354)
  feat(tpu-v2): provide swap protocol versioning (GLEECBTC#2324)
  feat(wallet): add change mnemonic password rpc (GLEECBTC#2317)
  fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret (GLEECBTC#2261)
  feat(tendermint): unstaking/undelegation (GLEECBTC#2330)
  fix(utxo-withdraw): get hw ctx only when `PrivKeyPolicy` is trezor (GLEECBTC#2333)
  feat(event-streaming): API-driven subscription management (GLEECBTC#2172)
  fix(hash-types): remove panic, enforce fixed-size arrays (GLEECBTC#2279)
  fix(ARRR): store unconfirmed change output (GLEECBTC#2276)
  feat(tendermint): staking/delegation (GLEECBTC#2322)
  chore(deps): `timed-map` migration (GLEECBTC#2247)
  fix(mem-leak): `running_swap` never shrinks (GLEECBTC#2301)
  ...
dimxy pushed a commit that referenced this pull request Mar 5, 2025
Merge branch 'dev' into burn-dex-fee-for-non-kmd
356eaad 
* dev:
  feat(rpc): add is_success field to legacy MySwapStatusResponse (#2371)
  fix(key-derivation): use stored Argon2 parameters instead of default values (#2360)
  fix(tests): stabilize `tendermint_coin::test_claim_staking_rewards` (#2373)
  improvement(RPCs): group staking rpcs under a namespace (#2372)
  feat(tendermint): claim delegation rewards (#2351)
  fix(eth-tpu): remove state from funding validation (#2334)
  improvement(rpc-server): rpc server dynamic port allocation (#2342)
  fix(tests): fix or ignore unstable tests (#2365)
  fix(fs): make `filter_files_by_extension` return only files (#2364)
  fix(derive_key_from_path): check length of current_key_material (#2356)
  chore(release): bump mm2 version to 2.4.0-beta (#2346)
  fix(tests): add additional testnet sepolia nodes to test code (#2358)
  fix(swaps): maintain legacy compatibility for negotiation messages (#2353)
  refactor(SwapOps): impl defaults for protocol specific swapops fns (#2354)
  feat(tpu-v2): provide swap protocol versioning (#2324)
  feat(wallet): add change mnemonic password rpc (#2317)
@shamardy shamardy mentioned this pull request Apr 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smk762 smk762 Awaiting requested review from smk762

3 more reviewers

@cipig cipig cipig approved these changes

@borngraced borngraced borngraced approved these changes

@onur-ozkan onur-ozkan onur-ozkan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status: pending review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shamardy @cipig @onur-ozkan @borngraced