Introduce `TxInput::OrderAccountCommand` without nonces #1888

azarovh · 2025-02-28T11:25:04Z

Filled amount is now calculated based on the original ask/give price. This makes fill operations commutative. Though it has a negative consequence as potential remainder amounts, resulting in dust utxos produced on conclude, I don't think it's a major problem.
It is forbidden to fill order with 0 to enforce spending utxo in a tx making it unique and not replayable.
Nonce for AccountCommand::FillOrder is ignored. This is a bit hacky because nonce is still present making it confusing. But I think that this is more like an exception and refactoring out nonce from TxInput::AccountCommand seems like not worth it.
~~Nonce for AccountCommand::ConcludeOrder is kept. Also I'm planning to keep it for the upcoming AccountCommand::FreezeOrder. Though it seems like it can be removed I don't see a point in doing it.~~
~~Ignoring nonce forced me to add fork logic to TransactionVerifier::disconnect_transaction function because we don't need to decrement nonce for fill orders after the fork.~~
~~Same fork logic goes into api-server and wallet. Though for wallet it required changing OutputCache logic which looks messy and I'm don't quite like it (alternative solution below can fix that).~~

UPDATE:

Introduced new TxInput::OrderAccountCommand with FillOrder and ConcludeOrder commands.
Nonces for OrderAccountCommand are completely removed.
OrderAccountCommand::ConcludeOrder now contains remaining amounts that can be withdrawn from an account so that these amount can be used by hw wallet and also get into a signature. Also these amounts are checked in TransactionVerifier

azarovh · 2025-03-18T10:56:50Z

common/src/chain/transaction/account_outpoint.rs

+        filled_amount: Amount,
+        remaining_give_amount: Amount,


I will remove these fields if we go with OrderData in the signature

Seems like I cannot do that because OrderData does not contain current balances

ImplOfAnImpl

Do we have a test for the case when an order is created and partially filled before the upgrade and then filled again and concluded after?

ImplOfAnImpl · 2025-03-24T14:21:53Z

api-server/scanner-lib/src/blockchain_state/mod.rs

+        let ask = match order.ask_currency {
+            CoinOrTokenId::Coin => OutputValue::Coin(order.initially_asked),
+            CoinOrTokenId::TokenId(token_id) => {
+                OutputValue::TokenV1(token_id, order.initially_asked)
+            }
+        };
+        let give = match order.give_currency {
+            CoinOrTokenId::Coin => OutputValue::Coin(order.initially_given),
+            CoinOrTokenId::TokenId(token_id) => {
+                OutputValue::TokenV1(token_id, order.initially_given)
+            }
+        };


Add method fn from_output_value(&self, Amount) to CoinOrTokenId?

ImplOfAnImpl · 2025-03-24T14:29:56Z

api-server/scanner-lib/src/blockchain_state/mod.rs

+                        match order.ask_currency {
+                            CoinOrTokenId::Coin => {}
+                            CoinOrTokenId::TokenId(id) => {
+                                token_ids.insert(id);
+                            }
+                        };


This match is repeated in many places. Maybe add a method CoinOrTokenId::token_id(&self) -> Option<TokenId>?

Or maybe 2 separate ones: token_id(&self) -> Option<&TokenId> and into_token_id(self) -> Option<TokenId>.

ImplOfAnImpl · 2025-03-24T14:31:31Z

api-server/scanner-lib/src/sync/tests/simulation.rs

@@ -646,6 +646,7 @@ fn update_statistics(
            }
            AccountCommand::ConcludeOrder(_) | AccountCommand::FillOrder(_, _, _) => {}
        },
+        TxInput::OrderAccountCommand(..) => {}


So I guess these tests don't currently use OrderAccountCommand?

ImplOfAnImpl · 2025-03-24T14:35:59Z

api-server/stack-test-suite/tests/v2/orders.rs

                    .add_input(
-                        TxInput::AccountCommand(
-                            AccountNonce::new(0),
-                            AccountCommand::FillOrder(
-                                order_id,
-                                Amount::from_atoms(1),
-                                Destination::AnyoneCanSpend,
-                            ),
-                        ),
+                        TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
+                            order_id,


Why not parameterize this test to check both old and new commands?

ImplOfAnImpl · 2025-03-24T14:44:12Z

api-server/web-server/src/api/json_helpers.rs

+                    "command": "FillOrder",
+                    "order_id": Address::new(chain_config, *id).expect("addressable").to_string(),
+                    "fill_atoms": json!({"atoms": fill.into_atoms().to_string()}),
+                    "destination": Address::new(chain_config, dest.clone()).expect("no error").as_str(),


IMO we should try to get rid of all these optimistic expects in the API server and propagate errors correctly.
A good rule might be that if you're going to add a new "expect", you should consider changing the function to return a proper Result and fix the existing "expect"s as well.

I'm not insisting on doing it right now, just a thing to consider.

(If you keep the "expect"s, plz at least use consistent messages)

ImplOfAnImpl · 2025-03-24T17:57:05Z

orders-accounting/src/error.rs

@@ -47,6 +47,8 @@ pub enum Error {
    OrderOverflow(OrderId),
    #[error("Order `{0}` can provide `{1:?}` amount; but attempted to fill `{2:?}`")]
    OrderOverbid(OrderId, Amount, Amount),
+    #[error("Order `{0}` provides amount `{1:?}` that is not enough to fill even a single coin")]


coin -> atom?

ImplOfAnImpl · 2025-03-24T18:04:20Z

trezor-common/src/tests/mod.rs

@@ -146,6 +146,10 @@ impl From<chain::TxInput> for crate::TxInput {
            chain::TxInput::AccountCommand(nonce, command) => {
                Self::AccountCommand(nonce.value(), command.into())
            }
+            chain::TxInput::OrderAccountCommand(_) => {
+                //TODO: support OrdersVersion::V1


I guess this should be put into #1887

Or maybe some other issue

ImplOfAnImpl · 2025-03-24T18:19:05Z

wallet/src/account/mod.rs

+        let version = self
+            .chain_config
+            .chainstate_upgrades()
+            .version_at_height(self.account_info.best_block_height())
+            .1
+            .orders_version();
+


Tecnically, this can be "best_block_height + 1", because when the tx is mined, the upgrade height will be reached. (Unless a reorg occurs and the tx gets mined at a lower height, but I guess this is always possible).

ImplOfAnImpl · 2025-03-24T18:36:24Z

wallet/src/account/mod.rs

+    let filled_amount = orders_accounting::calculate_filled_amount(
+        order_info.ask_balance,


Probably it's better to use initially_asked/initially_given here and/or assert that they equal ask_balance/give_balance?

ImplOfAnImpl · 2025-03-24T18:43:21Z

wallet/wallet-controller/src/helpers.rs

@@ -291,42 +291,13 @@ async fn into_utxo_and_destination<T: NodeInterface, B: storage::Backend>(
            (None, TxAdditionalInfo::new(), dest)
        }
        TxInput::AccountCommand(_, cmd) => {
-            // find authority of the token
+            // find authority of the order


The comment is still weird.

"find a destination associated with the order" would make more sense (not sure if it's useful though)

azarovh force-pushed the fix/fill_order_no_nonce branch from fcf7fc6 to b6d9b5d Compare March 4, 2025 15:58

azarovh added 16 commits March 5, 2025 11:50

Forbid filling order with zero amount

bef71a2

New fill calculation logic

4e26298

Chainstate upgrade builder

fa881ef

Define orders fork

36d9e08

Introduce fork and underbid

b83e974

Ignore nonces for fill orders

5343ebf

Provide block height for tx disconnect

c78bc0b

Handle fill nonces in the wallet

df47aea

Handle both versions in api-server

4a81f01

Introduce OrdersAccountCommand

ad3e38d

Cover order command in chainstate

3061854

Revert forking AccountCommand::FillOrder

384a8cb

Fix tests

2a29a48

Stub the wallet

f640306

Check ConcludeOrder amounts

3e99975

Handle fork in the wallet

Loading
Loading status checks…

461b8fa

azarovh force-pushed the fix/fill_order_no_nonce branch from 6e4bf6f to 461b8fa Compare March 5, 2025 10:44

azarovh changed the title ~~Ignore nonces in fill order operations~~ Introduce TxInput::OrderAccountCommand without nonces Mar 5, 2025

azarovh force-pushed the fix/fill_order_no_nonce branch from b558f29 to 8c03c73 Compare March 5, 2025 13:35

azarovh marked this pull request as ready for review March 5, 2025 13:45

azarovh requested review from erubboli, ImplOfAnImpl and OBorce as code owners March 5, 2025 13:45

Minor cleanup

Loading
Loading status checks…

816b30b

azarovh force-pushed the fix/fill_order_no_nonce branch from 8c03c73 to 816b30b Compare March 5, 2025 15:41

azarovh added 2 commits March 10, 2025 16:05

Fix underbidding in simulation

1962093

Additional tests

Loading
Loading status checks…

b8a6529

azarovh force-pushed the fix/fill_order_no_nonce branch from 7791573 to b8a6529 Compare March 10, 2025 14:05

azarovh commented Mar 18, 2025

View reviewed changes

ImplOfAnImpl approved these changes Mar 24, 2025

View reviewed changes

azarovh marked this pull request as draft March 25, 2025 08:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `TxInput::OrderAccountCommand` without nonces #1888

Introduce `TxInput::OrderAccountCommand` without nonces #1888

azarovh commented Feb 28, 2025 •

edited

Loading

azarovh Mar 18, 2025 •

edited

Loading

azarovh Mar 18, 2025

ImplOfAnImpl left a comment

ImplOfAnImpl Mar 24, 2025

ImplOfAnImpl Mar 24, 2025

ImplOfAnImpl Mar 24, 2025

ImplOfAnImpl Mar 24, 2025

ImplOfAnImpl Mar 24, 2025

ImplOfAnImpl Mar 24, 2025

ImplOfAnImpl Mar 24, 2025

ImplOfAnImpl Mar 24, 2025

ImplOfAnImpl Mar 24, 2025

ImplOfAnImpl Mar 24, 2025

		let filled_amount = orders_accounting::calculate_filled_amount(
		order_info.ask_balance,

Introduce TxInput::OrderAccountCommand without nonces #1888

Are you sure you want to change the base?

Introduce TxInput::OrderAccountCommand without nonces #1888

Conversation

azarovh commented Feb 28, 2025 • edited Loading

azarovh Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ImplOfAnImpl 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

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

Introduce `TxInput::OrderAccountCommand` without nonces #1888

Introduce `TxInput::OrderAccountCommand` without nonces #1888

azarovh commented Feb 28, 2025 •

edited

Loading

azarovh Mar 18, 2025 •

edited

Loading