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

Introduce TxInput::OrderAccountCommand without nonces #1888

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from
Draft

Introduce TxInput::OrderAccountCommand without nonces #1888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
bef71a2
Forbid filling order with zero amount
azarovh Feb 20, 2025
4e26298
New fill calculation logic
azarovh Feb 21, 2025
fa881ef
Chainstate upgrade builder
azarovh Feb 21, 2025
36d9e08
Define orders fork
azarovh Feb 21, 2025
b83e974
Introduce fork and underbid
azarovh Feb 24, 2025
5343ebf
Ignore nonces for fill orders
azarovh Feb 25, 2025
c78bc0b
Provide block height for tx disconnect
azarovh Feb 26, 2025
df47aea
Handle fill nonces in the wallet
azarovh Feb 26, 2025
4a81f01
Handle both versions in api-server
azarovh Feb 28, 2025
ad3e38d
Introduce OrdersAccountCommand
azarovh Mar 3, 2025
3061854
Cover order command in chainstate
azarovh Mar 3, 2025
384a8cb
Revert forking AccountCommand::FillOrder
azarovh Mar 4, 2025
2a29a48
Fix tests
azarovh Mar 4, 2025
f640306
Stub the wallet
azarovh Mar 4, 2025
3e99975
Check ConcludeOrder amounts
azarovh Mar 4, 2025
461b8fa
Handle fork in the wallet
azarovh Mar 5, 2025
816b30b
Minor cleanup
azarovh Mar 5, 2025
1962093
Fix underbidding in simulation
azarovh Mar 6, 2025
b8a6529
Additional tests
azarovh Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions api-server/api-server-common/src/storage/storage_api/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,10 +194,24 @@ pub struct Order {
}

impl Order {
pub fn fill(self, fill_amount_in_ask_currency: Amount) -> Self {
pub fn fill(
self,
chain_config: &ChainConfig,
block_height: BlockHeight,
fill_amount_in_ask_currency: Amount,
) -> Self {
let (ask_balance, give_balance) = match chain_config
.chainstate_upgrades()
.version_at_height(block_height)
.1
.orders_version()
{
common::chain::OrdersVersion::V0 => (self.ask_balance, self.give_balance),
common::chain::OrdersVersion::V1 => (self.initially_asked, self.initially_given),
};
let filled_amount = orders_accounting::calculate_filled_amount(
self.ask_balance,
self.give_balance,
ask_balance,
give_balance,
fill_amount_in_ask_currency,
)
.expect("must succeed");
Expand Down
102 changes: 84 additions & 18 deletions api-server/scanner-lib/src/blockchain_state/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ use common::{
tokens::{get_referenced_token_ids, make_token_id, IsTokenFrozen, TokenId, TokenIssuance},
transaction::OutPointSourceId,
AccountCommand, AccountNonce, AccountSpending, Block, DelegationId, Destination, GenBlock,
Genesis, OrderData, OrderId, PoolId, SignedTransaction, Transaction, TxInput, TxOutput,
UtxoOutPoint,
Genesis, OrderAccountCommand, OrderData, OrderId, PoolId, SignedTransaction, Transaction,
TxInput, TxOutput, UtxoOutPoint,
},
primitives::{id::WithId, Amount, BlockHeight, CoinOrTokenId, Fee, Id, Idable, H256},
};
Expand Down Expand Up @@ -631,6 +631,28 @@ async fn calculate_tx_fee_and_collect_token_info<T: ApiServerStorageWrite>(
};
}
},
TxInput::OrderAccountCommand(cmd) => match cmd {
OrderAccountCommand::FillOrder(order_id, _, _)
| OrderAccountCommand::ConcludeOrder {
order_id,
filled_amount: _,
remaining_give_amount: _,
} => {
let order = db_tx.get_order(*order_id).await?.expect("must exist");
match order.ask_currency {
CoinOrTokenId::Coin => {}
CoinOrTokenId::TokenId(id) => {
token_ids.insert(id);
}
};
Comment on lines +642 to +647
Copy link
Contributor

Choose a reason for hiding this comment

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

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>.

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

Expand Down Expand Up @@ -675,7 +697,9 @@ async fn fetch_utxo<T: ApiServerStorageRead>(
.expect("must be present");
Ok(Some(utxo))
}
TxInput::Account(_) | TxInput::AccountCommand(_, _) => Ok(None),
TxInput::Account(_) | TxInput::AccountCommand(_, _) | TxInput::OrderAccountCommand(_) => {
Ok(None)
}
}
}

Expand Down Expand Up @@ -842,9 +866,40 @@ async fn prefetch_orders<T: ApiServerStorageRead>(
let mut ask_balances = BTreeMap::<OrderId, Amount>::new();
let mut give_balances = BTreeMap::<OrderId, Amount>::new();

let to_order_data = |order: &Order| {
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)
}
};
Comment on lines +870 to +881
Copy link
Contributor

Choose a reason for hiding this comment

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

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

OrderData::new(order.conclude_destination.clone(), ask, give)
};

for input in inputs {
match input {
TxInput::Utxo(_) | TxInput::Account(_) => {}
TxInput::OrderAccountCommand(cmd) => match cmd {
OrderAccountCommand::FillOrder(order_id, _, _)
| OrderAccountCommand::ConcludeOrder {
order_id,
filled_amount: _,
remaining_give_amount: _,
} => {
let order = db_tx.get_order(*order_id).await?.expect("must be present ");
let order_data = to_order_data(&order);

ask_balances.insert(*order_id, order.ask_balance);
give_balances.insert(*order_id, order.give_balance);
orders_data.insert(*order_id, order_data);
}
},
TxInput::AccountCommand(_, account_command) => match account_command {
AccountCommand::MintTokens(_, _)
| AccountCommand::UnmintTokens(_)
Expand All @@ -856,21 +911,10 @@ async fn prefetch_orders<T: ApiServerStorageRead>(
AccountCommand::FillOrder(order_id, _, _)
| AccountCommand::ConcludeOrder(order_id) => {
let order = db_tx.get_order(*order_id).await?.expect("must be present ");
let order_data = to_order_data(&order);

ask_balances.insert(*order_id, order.ask_balance);
give_balances.insert(*order_id, order.give_balance);
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)
}
};
let order_data = OrderData::new(order.conclude_destination.clone(), ask, give);
orders_data.insert(*order_id, order_data);
}
},
Expand Down Expand Up @@ -922,7 +966,9 @@ async fn update_tables_from_consensus_data<T: ApiServerStorageWrite>(
)
.await;
}
TxInput::Account(_) | TxInput::AccountCommand(_, _) => {}
TxInput::Account(_)
| TxInput::AccountCommand(_, _)
| TxInput::OrderAccountCommand(_) => {}
}
}

Expand Down Expand Up @@ -1215,7 +1261,8 @@ async fn update_tables_from_transaction_inputs<T: ApiServerStorageWrite>(
}
AccountCommand::FillOrder(order_id, fill_amount_in_ask_currency, _) => {
let order = db_tx.get_order(*order_id).await?.expect("must exist");
let order = order.fill(*fill_amount_in_ask_currency);
let order =
order.fill(&chain_config, block_height, *fill_amount_in_ask_currency);

db_tx.set_order_at_height(*order_id, &order, block_height).await?;
}
Expand All @@ -1226,6 +1273,25 @@ async fn update_tables_from_transaction_inputs<T: ApiServerStorageWrite>(
db_tx.set_order_at_height(*order_id, &order, block_height).await?;
}
},
TxInput::OrderAccountCommand(cmd) => match cmd {
OrderAccountCommand::FillOrder(order_id, fill_amount_in_ask_currency, _) => {
let order = db_tx.get_order(*order_id).await?.expect("must exist");
let order =
order.fill(&chain_config, block_height, *fill_amount_in_ask_currency);

db_tx.set_order_at_height(*order_id, &order, block_height).await?;
}
OrderAccountCommand::ConcludeOrder {
order_id,
filled_amount: _,
remaining_give_amount: _,
} => {
let order = db_tx.get_order(*order_id).await?.expect("must exist");
let order = order.conclude();

db_tx.set_order_at_height(*order_id, &order, block_height).await?;
}
},
TxInput::Account(outpoint) => {
match outpoint.account() {
AccountSpending::DelegationBalance(delegation_id, amount) => {
Expand Down
1 change: 1 addition & 0 deletions api-server/scanner-lib/src/sync/tests/simulation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,7 @@ fn update_statistics(
}
AccountCommand::ConcludeOrder(_) | AccountCommand::FillOrder(_, _, _) => {}
},
TxInput::OrderAccountCommand(..) => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

});
}

Expand Down
4 changes: 3 additions & 1 deletion api-server/stack-test-suite/tests/v2/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,9 @@ async fn get_tx_additional_data(
TxInput::Utxo(outpoint) => {
db_tx.get_utxo(outpoint.clone()).await.unwrap().map(|utxo| utxo.into_output())
}
TxInput::Account(_) | TxInput::AccountCommand(_, _) => None,
TxInput::Account(_)
| TxInput::AccountCommand(_, _)
| TxInput::OrderAccountCommand(_) => None,
};
input_utxos.push(utxo);
}
Expand Down
24 changes: 11 additions & 13 deletions api-server/stack-test-suite/tests/v2/orders.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use common::chain::{make_order_id, AccountCommand, AccountNonce, OrderData};
use common::chain::{make_order_id, OrderAccountCommand, OrderData};

use super::*;

Expand Down Expand Up @@ -68,14 +68,11 @@ async fn create_fill_conclude_order(#[case] seed: Seed) {
InputWitness::NoSignature(None),
)
.add_input(
TxInput::AccountCommand(
AccountNonce::new(0),
AccountCommand::FillOrder(
order_id,
Amount::from_atoms(1),
Destination::AnyoneCanSpend,
),
),
TxInput::OrderAccountCommand(OrderAccountCommand::FillOrder(
order_id,
Comment on lines 70 to +72
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Amount::from_atoms(1),
Destination::AnyoneCanSpend,
)),
InputWitness::NoSignature(None),
)
.add_output(TxOutput::Transfer(
Expand All @@ -91,10 +88,11 @@ async fn create_fill_conclude_order(#[case] seed: Seed) {
// Conclude order
let tx3 = TransactionBuilder::new()
.add_input(
TxInput::AccountCommand(
AccountNonce::new(1),
AccountCommand::ConcludeOrder(order_id),
),
TxInput::OrderAccountCommand(OrderAccountCommand::ConcludeOrder {
order_id,
filled_amount: Amount::from_atoms(1),
remaining_give_amount: Amount::from_atoms(9),
}),
InputWitness::NoSignature(None),
)
.add_output(TxOutput::Transfer(
Expand Down
8 changes: 6 additions & 2 deletions api-server/stack-test-suite/tests/v2/transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,9 @@ async fn multiple_tx_in_same_block(#[case] seed: Seed) {
TxInput::Utxo(outpoint) => {
Some(signed_tx1.outputs()[outpoint.output_index() as usize].clone())
}
TxInput::Account(_) | TxInput::AccountCommand(_, _) => None,
TxInput::Account(_)
| TxInput::AccountCommand(_, _)
| TxInput::OrderAccountCommand(_) => None,
});

let transaction = signed_tx2.transaction();
Expand Down Expand Up @@ -327,7 +329,9 @@ async fn ok(#[case] seed: Seed) {
TxInput::Utxo(outpoint) => {
Some(prev_tx.outputs()[outpoint.output_index() as usize].clone())
}
TxInput::Account(_) | TxInput::AccountCommand(_, _) => None,
TxInput::Account(_)
| TxInput::AccountCommand(_, _)
| TxInput::OrderAccountCommand(_) => None,
});

let expected_transaction = json!({
Expand Down
4 changes: 3 additions & 1 deletion api-server/stack-test-suite/tests/v2/transactions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,9 @@ async fn ok(#[case] seed: Seed) {
TxInput::Utxo(outpoint) => Some(
prev_tx.outputs()[outpoint.output_index() as usize].clone(),
),
TxInput::Account(_) | TxInput::AccountCommand(_, _) => None,
TxInput::Account(_)
| TxInput::AccountCommand(_, _)
| TxInput::OrderAccountCommand(_) => None,
})
.collect();

Expand Down
3 changes: 2 additions & 1 deletion api-server/storage-test-suite/src/basic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1430,6 +1430,7 @@ async fn orders<'a, S: for<'b> Transactional<'b>>(
rng: &mut (impl Rng + CryptoRng),
storage: &'a mut S,
) {
let chain_config = common::chain::config::create_regtest();
{
let db_tx = storage.transaction_ro().await.unwrap();
let random_order_id = OrderId::new(H256::random_using(rng));
Expand Down Expand Up @@ -1542,7 +1543,7 @@ async fn orders<'a, S: for<'b> Transactional<'b>>(
);

// Fill one order
let order2_filled = order2.clone().fill(Amount::from_atoms(1));
let order2_filled = order2.clone().fill(&chain_config, block_height, Amount::from_atoms(1));
db_tx
.set_order_at_height(order2_id, &order2_filled, block_height.next_height())
.await
Expand Down
28 changes: 26 additions & 2 deletions api-server/web-server/src/api/json_helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ use common::{
block::ConsensusData,
output_value::OutputValue,
tokens::{IsTokenUnfreezable, NftIssuance, TokenId, TokenTotalSupply},
AccountCommand, AccountSpending, Block, ChainConfig, Destination, OrderId,
OutPointSourceId, Transaction, TxInput, TxOutput, UtxoOutPoint,
AccountCommand, AccountSpending, Block, ChainConfig, Destination, OrderAccountCommand,
OrderId, OutPointSourceId, Transaction, TxInput, TxOutput, UtxoOutPoint,
},
primitives::{Amount, BlockHeight, CoinOrTokenId, Idable},
Uint256,
Expand Down Expand Up @@ -318,6 +318,30 @@ pub fn tx_input_to_json(inp: &TxInput, chain_config: &ChainConfig) -> serde_json
})
}
},
TxInput::OrderAccountCommand(cmd) => match cmd {
OrderAccountCommand::FillOrder(id, fill, dest) => {
json!({
"input_type": "OrderAccountCommand",
"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(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

})
}
OrderAccountCommand::ConcludeOrder {
order_id,
filled_amount,
remaining_give_amount,
} => {
json!({
"input_type": "OrderAccountCommand",
"command": "ConcludeOrder",
"order_id": Address::new(chain_config, *order_id).expect("addressable").to_string(),
"filled_atoms": json!({"atoms": filled_amount.into_atoms().to_string()}),
"remaining_give_atoms": json!({"atoms": remaining_give_amount.into_atoms().to_string()}),
})
}
},
TxInput::AccountCommand(nonce, cmd) => match cmd {
AccountCommand::MintTokens(token_id, amount) => {
json!({
Expand Down
Loading