Skip to content

feat(orderbook): expirable maker orders#2516

Merged
shamardy merged 8 commits intodevfrom
expirable-orders
Jul 8, 2025
Merged

feat(orderbook): expirable maker orders#2516
shamardy merged 8 commits intodevfrom
expirable-orders

Conversation

@onur-ozkan
Copy link
Copy Markdown

@onur-ozkan onur-ozkan commented Jul 2, 2025

Implements expirable maker orders which can be easily used with timeout_in_minutes arg in setprice RPC. It's an optional field and doesn't cause a breaking-change.

Changes on the 1.5.0 timed-map release: Orkavian/timed-map#12
Documentation issue: GLEECBTC/komodo-docs-mdx#537
Closes #2479

Signed-off-by: Onur Özkan <work@onurozkan.dev>
Signed-off-by: Onur Özkan <work@onurozkan.dev>
Signed-off-by: Onur Özkan <work@onurozkan.dev>
Signed-off-by: Onur Özkan <work@onurozkan.dev>
Signed-off-by: Onur Özkan <work@onurozkan.dev>
Signed-off-by: Onur Özkan <work@onurozkan.dev>
@shamardy
Copy link
Copy Markdown
Collaborator

shamardy commented Jul 3, 2025

@onur-ozkan test_expirable_order fails in CI and locally.

Signed-off-by: Onur Özkan <work@onurozkan.dev>
@onur-ozkan
Copy link
Copy Markdown
Author

Should be fixed now, thanks.

@shamardy shamardy requested a review from cipig July 3, 2025 09:09
@shamardy
Copy link
Copy Markdown
Collaborator

shamardy commented Jul 3, 2025

@cipig please take a look at this Pr and see if it matches what you needed

Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

First review iteration from my side. I also have a question, what happens on restart, do we check expirations of saved orders before rebroadcasting them?

rel_nota: Option<bool>,
#[serde(default = "get_true")]
save_in_history: bool,
timeout_in_minutes: Option<u16>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why minutes not seconds?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think using seconds would be unnecessary and can be bad for the network stability (imagine people sending orders with ~5-10 seconds of expiration time).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Makes sense, will wait for @cipig input since he requested this

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

imagine people sending orders with ~5-10 seconds of expiration time

On the other hand, if or when we can support this, it won't be bad as it will mirror the CEX experience a lot.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And they can cancel it in whatever seconds they like manually or using scripts

Copy link
Copy Markdown
Author

@onur-ozkan onur-ozkan Jul 3, 2025

Choose a reason for hiding this comment

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

It wouldn't be harmful on CEX platforms because spamming very short-lived orders doesn't affect other nodes. But on a P2P network I think orders should live for at least 30 seconds or so. For example orders that last only 5 seconds just put pressure on the network, especially since it can take up to 5 seconds for peers to fully sync. Instead doing manual validations on the expiration time, I decided to make it minute and I tought it's quite reasonable to say "orders must have at least 1 minute expiration time" to users.

Also, when taking so many users into account, with seconds, this feature alone can put quite a lot of pressure on the network, with minutes, it should be way less compare to seconds.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I understand and agree. I was just arguing the opposite case, I plan to leave it at minutes unless @cipig objects and wants something like 30 seconds.

Copy link
Copy Markdown
Collaborator

@shamardy shamardy Jul 3, 2025

Choose a reason for hiding this comment

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

Cancelling in seconds doesn't have to have an immediate p2p message propagated but if it's matched before it's cancelled on remote nodes, the maker node will not proceed with the order when it's not available locally. And takers choose between the best response from multiple makers anyway so it won't cause problems. But that's for another PR or issue if needed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Makes sense, will wait for @cipig input since he requested this

Minutes is perfectly fine. I added it in my bot with a 20 minute timeout (cipig/mmtools@5ee5dbe). It needs anyway 5-15 minutes to update all orders, depending on number of coins. If the bot dies for whatever reason, orders are cancelled like this:

22 14:32:03, mm2_main::lp_ordermatch:3591] INFO Order 'afb833e8-7d6a-42da-9151-4f8f096efb57' is expired, cancelling
22 14:32:05, mm2_main::lp_ordermatch:3591] INFO Order 'f1cc74c7-b0f0-40d7-a4a6-036ea66a39e2' is expired, cancelling
22 14:32:07, mm2_main::lp_ordermatch:3591] INFO Order 'fc996c71-0d19-4e7c-bc35-636afdd88d22' is expired, cancelling
22 14:32:09, mm2_main::lp_ordermatch:3591] INFO Order '28088d79-694b-4b84-965b-1c97b0827729' is expired, cancelling

That's exactly what i wanted, as a safety measure for my own bad programming :-)
Thanks a lot.

@shamardy shamardy merged commit 805aed3 into dev Jul 8, 2025
18 of 25 checks passed
@shamardy shamardy deleted the expirable-orders branch July 8, 2025 10:55
dimxy pushed a commit that referenced this pull request Jul 13, 2025
* dev:
  fix(eth): Correctly implement ETH max withdrawal logic (#2531)
  feat(use-clap-for-cli): use clap to parse CLI-Args #2215 (#2510)
  feat(orderbook): expirable maker orders (#2516)
  improvement(eth): drop parity support (#2527)
  chore(release): finalize changelog for v2.5.0-beta (#2524)
  chore(toolchain): upgrade toolchain to nightly 1.86.0 (#2444)
  feat(swap): rpc to find best swap with liquidity routing for ask (#2362)
  fix(kdf_walletconnect): apply explicit MmError mapping (#2514)
  fix(walletconnect): centralize connection and retry logic (#2508)
  fix(hw-wallet): avoid calling `get_enabled_address` in trezor-based coin init (#2504)
dimxy pushed a commit to dimxy/komodo-defi-framework that referenced this pull request Jul 16, 2025
* refactor-gas-fee-policy: (30 commits)
  revert change in test_enable_custom_erc20
  fix(test) disable get_swap_gas_fee_policy for tests (no platform coin in tests)
  revert tokio test and use block_on for eth docker test
  Refactor gas fee code: get max_eth_tx_type from platform coin, add gas fee adjust coin params, rename "set_swap_transaction_fee_policy" rpc to "set_swap_gas_fee_policy", force set_swap_gas_fee_policy to store policy in the platform coin
  fix(eth): Propagate structured EIP-1559 fee errors (GLEECBTC#2532)
  fix(eth): Correctly implement ETH max withdrawal logic (GLEECBTC#2531)
  feat(use-clap-for-cli): use clap to parse CLI-Args GLEECBTC#2215 (GLEECBTC#2510)
  feat(orderbook): expirable maker orders (GLEECBTC#2516)
  improvement(eth): drop parity support (GLEECBTC#2527)
  chore(release): finalize changelog for v2.5.0-beta (GLEECBTC#2524)
  chore(toolchain): upgrade toolchain to nightly 1.86.0 (GLEECBTC#2444)
  feat(swap): rpc to find best swap with liquidity routing for ask (GLEECBTC#2362)
  fix(kdf_walletconnect): apply explicit MmError mapping (GLEECBTC#2514)
  fix(walletconnect): centralize connection and retry logic (GLEECBTC#2508)
  fix(hw-wallet): avoid calling `get_enabled_address` in trezor-based coin init (GLEECBTC#2504)
  chore(core): replace hash_raw_entry with stable entry() API (GLEECBTC#2473)
  chore(core): adapt `MmError` and usages for compatibility with new rustc versions (GLEECBTC#2443)
  feat(wallet): add `delete_wallet` RPC (GLEECBTC#2497)
  chore(release): add changelog entries for v2.5.0-beta (GLEECBTC#2494)
  chore(release): bump kdf version to 2.5.0-beta (GLEECBTC#2492)
  ...

# Conflicts:
#	mm2src/coins/eth.rs
#	mm2src/coins/eth/eth_swap_v2/eth_maker_swap_v2.rs
#	mm2src/coins/eth/eth_swap_v2/eth_taker_swap_v2.rs
#	mm2src/coins/eth/eth_utils.rs
#	mm2src/coins/eth/eth_withdraw.rs
#	mm2src/coins/eth/v2_activation.rs
#	mm2src/coins/lp_coins.rs
#	mm2src/coins/qrc20.rs
#	mm2src/coins/qrc20/swap.rs
#	mm2src/coins_activation/src/platform_coin_with_tokens.rs
#	mm2src/coins_activation/src/token.rs
#	mm2src/mm2_main/src/lp_swap/check_balance.rs
#	mm2src/mm2_main/src/lp_swap/maker_swap.rs
#	mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs
#	mm2src/mm2_main/src/rpc/lp_commands/ext_api/ext_api_types.rs
#	mm2src/mm2_main/src/rpc/lp_commands/mod.rs
#	mm2src/mm2_main/src/rpc/lp_commands/one_inch/errors.rs
#	mm2src/mm2_main/src/rpc/lp_commands/one_inch/rpcs.rs
#	mm2src/mm2_main/src/rpc/lp_commands/tokens.rs
#	mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs
#	mm2src/trading_api/src/one_inch_api/classic_swap_types.rs
#	mm2src/trading_api/src/one_inch_api/client.rs
#	mm2src/trading_api/src/one_inch_api/portfolio_types.rs
@cipig
Copy link
Copy Markdown

cipig commented Jul 22, 2025

one last question: if not too much work, can we add more infos to the INFO Order 'e589e874-cac1-4680-9203-e4177a9f24cb' is expired, cancelling output? I see it popping up sometimes and would like to know which pair that is. I have set "save_in_history":false in my setprice, for performance reasons, so i don't have a history of orders to check.

dimxy pushed a commit that referenced this pull request Aug 1, 2025
* dev: (21 commits)
  feat(wallet-connect): impl BTC (UTxO) activation via WalletConnect (#2499)
  feat(utxo): add new fixed txfee option for DINGO-like coins (#2454)
  ci(pull-requests): review notification bot (#2468)
  improvement(walletconnect): return the `pairing_topic` in `new_connection` response (#2538)
  bless clippy (#2560)
  refactor(toolchain): use latest available stable compiler (#2557)
  feat(wallet): implement unified offline private key export API (#2542)
  improve note for docker test start failure (#2550)
  fix(DOCS): add note for macos to fix docker containers startup failure (#2544)
  refactor(toolchain): general stabilization for stable rust (#2528)
  fix(ci): adds nodejs 20 to ci-container (#2536)
  fix(WASM and Debian): fix build failures (#2534)
  improvement(event-streaming): impl DeriveStreamerId trait for all streamers (#2489)
  fix(eth): Propagate structured EIP-1559 fee errors (#2532)
  fix(eth): Correctly implement ETH max withdrawal logic (#2531)
  feat(use-clap-for-cli): use clap to parse CLI-Args #2215 (#2510)
  feat(orderbook): expirable maker orders (#2516)
  improvement(eth): drop parity support (#2527)
  chore(release): finalize changelog for v2.5.0-beta (#2524)
  chore(toolchain): upgrade toolchain to nightly 1.86.0 (#2444)
  ...

# Conflicts:
#	mm2src/coins/lp_coins.rs
#	mm2src/coins/rpc_command/get_new_address.rs
#	mm2src/trezor/src/eth/eth_command.rs
dimxy pushed a commit that referenced this pull request Oct 15, 2025
Implements expirable maker orders which can be easily used with timeout_in_minutes arg in setprice RPC. It's an optional field and doesn't cause a breaking-change.
@smk762 smk762 added the status: pending docs Adding this label will automatically open an issue in the docs repo label Nov 26, 2025
@smk762
Copy link
Copy Markdown

smk762 commented Nov 26, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.6.0-beta feature: orders status: pending docs Adding this label will automatically open an issue in the docs repo status: pending review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants