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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Feb 12, 2025
Merged

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

merged 4 commits into from
Feb 12, 2025

Conversation

shamardy
Copy link
Collaborator

@shamardy shamardy mentioned this pull request Feb 11, 2025
Open
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
Collaborator Author

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

komodo-defi-framework/mm2src/mm2_main/src/lp_swap/taker_swap.rs

Line 660 in 39515a9

pub maker_pubkey: H264Json,

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

@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
Member

@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
Member

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
Collaborator Author

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

komodo-defi-framework/mm2src/mm2_main/src/lp_swap/maker_swap.rs

Lines 147 to 148 in 39515a9

#[serde(alias = "taker")]
pub taker_pubkey: H256Json,

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
Collaborator Author

@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
Member

@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
19 of 24 checks passed
@shamardy shamardy deleted the fix-neg-bc branch February 12, 2025 22:47
dimxy added a commit that referenced this pull request Feb 16, 2025
@dimxy
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 added a commit that referenced this pull request Feb 16, 2025
@dimxy
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 added a commit to dimxy/komodo-defi-framework that referenced this pull request Feb 24, 2025
@dimxy
Merge branch 'dev' into fix-utxo-tx-fee-for-change
534006e 
* dev: (24 commits)
  fix(eth-tpu): remove state from funding validation (KomodoPlatform#2334)
  improvement(rpc-server): rpc server dynamic port allocation (KomodoPlatform#2342)
  fix(tests): fix or ignore unstable tests (KomodoPlatform#2365)
  fix(fs): make `filter_files_by_extension` return only files (KomodoPlatform#2364)
  fix(derive_key_from_path): check length of current_key_material (KomodoPlatform#2356)
  chore(release): bump mm2 version to 2.4.0-beta (KomodoPlatform#2346)
  fix(tests): add additional testnet sepolia nodes to test code (KomodoPlatform#2358)
  fix(swaps): maintain legacy compatibility for negotiation messages (KomodoPlatform#2353)
  refactor(SwapOps): impl defaults for protocol specific swapops fns (KomodoPlatform#2354)
  feat(tpu-v2): provide swap protocol versioning (KomodoPlatform#2324)
  feat(wallet): add change mnemonic password rpc (KomodoPlatform#2317)
  fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret (KomodoPlatform#2261)
  feat(tendermint): unstaking/undelegation (KomodoPlatform#2330)
  fix(utxo-withdraw): get hw ctx only when `PrivKeyPolicy` is trezor (KomodoPlatform#2333)
  feat(event-streaming): API-driven subscription management (KomodoPlatform#2172)
  fix(hash-types): remove panic, enforce fixed-size arrays (KomodoPlatform#2279)
  fix(ARRR): store unconfirmed change output (KomodoPlatform#2276)
  feat(tendermint): staking/delegation (KomodoPlatform#2322)
  chore(deps): `timed-map` migration (KomodoPlatform#2247)
  fix(mem-leak): `running_swap` never shrinks (KomodoPlatform#2301)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cipig cipig cipig approved these changes

@borngraced borngraced borngraced approved these changes

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

@smk762 smk762 Awaiting requested review from smk762

Assignees
No one assigned
Labels
2.4.0-beta 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