Orders accounting #1740
Orders accounting #1740
Conversation
azarovh
force-pushed
the
feat/orders_accounting
branch
2 times, most recently
from
ea193a6
to
1feed11
Compare
wallet/src/account/mod.rs
Outdated
| // TODO: support orders | ||
| AccountCommand::WithdrawOrder(_) => unimplemented!(), | ||
| AccountCommand::FillOrder(_, _, _) => unimplemented!(), | ||
| } |
There was a problem hiding this comment.
Must be implemented before merging
wallet/src/account/mod.rs
Outdated
| // TODO: support orders | ||
| AccountCommand::WithdrawOrder(_) => unimplemented!(), | ||
| AccountCommand::FillOrder(_, _, _) => unimplemented!(), | ||
| }, |
There was a problem hiding this comment.
Must be implemented before merging
| @@ -2459,6 +2468,8 @@ fn group_preselected_inputs( | |||
| output.clone(), | |||
| ))) | |||
| } | |||
| // TODO: support orders | |||
| TxOutput::AnyoneCanTake(_) => unimplemented!(), | |||
There was a problem hiding this comment.
Must be implemented before merging
wallet/src/account/mod.rs
Outdated
| // TODO: support orders | ||
| AccountCommand::WithdrawOrder(_) => unimplemented!(), | ||
| AccountCommand::FillOrder(_, _, _) => unimplemented!(), | ||
| }, |
There was a problem hiding this comment.
Must be implemented before merging
| @@ -716,6 +719,8 @@ impl OutputCache { | |||
| | TxOutput::CreateDelegationId(_, _) | |||
| | TxOutput::IssueFungibleToken(_) | |||
| | TxOutput::ProduceBlockFromStake(_, _) => false, | |||
| // TODO: support orders | |||
| TxOutput::AnyoneCanTake(_) => unimplemented!(), | |||
There was a problem hiding this comment.
Must be implemented before merging
| AccountCommand::WithdrawOrder(_) => unimplemented!(), | ||
| AccountCommand::FillOrder(_, _, _) => unimplemented!(), | ||
| }, |
There was a problem hiding this comment.
Must be implemented before merging
There was a problem hiding this comment.
Many other unimplemented!() instances
| @@ -57,6 +57,8 @@ pub(crate) fn group_outputs<T, Grouped: Clone>( | |||
| get_tx_output(&output).clone(), | |||
| ))) | |||
| } | |||
| // TODO: support orders | |||
| TxOutput::AnyoneCanTake(_) => unimplemented!(), | |||
There was a problem hiding this comment.
Must be implemented before merging
| } | ||
|
|
||
| impl<P: OrdersAccountingView> OrdersAccountingOperations for OrdersAccountingCache<P> { | ||
| fn create_order(&mut self, id: OrderId, data: OrderData) -> Result<OrdersAccountingUndo> { |
There was a problem hiding this comment.
Reminder: Order id calculation should be automated + it should be enforced to use the automated results. We don't want people to be able to use any id they want.
There was a problem hiding this comment.
it is calculated in the tx-verifier based on tx input.
There was a problem hiding this comment.
Are you sure it's not possible to use a custom id? This is manually enforced for staking pools.
There was a problem hiding this comment.
It is possible to pass custom id to this function, e.g. in tests. But in transaction verifier it is called with id generated from utxo input.
For staking pool the situation is different because TxInput::CreateStakePool(PoolId, _) has pool id in it. So manual enforcement is required. But not for tokens and orders it is not the case.
There was a problem hiding this comment.
Please ensure this is tested, because abusing it can cause issues.
There was a problem hiding this comment.
Checked, it is tested in chainstate tests that an order is created with id produced from input0.
| tokens_accounting_cache: TokensAccountingCache<T>, | ||
| tokens_accounting_block_undo: AccountingBlockUndoCache<TokenAccountingUndo>, | ||
|
|
||
| orders_accounting_cache: OrdersAccountingCache<O>, | ||
| orders_accounting_block_undo: AccountingBlockUndoCache<OrdersAccountingUndo>, |
There was a problem hiding this comment.
All these should be somehow unified in the future.
azarovh
force-pushed
the
feat/orders_accounting
branch
from
8725475
to
4f53e00
Compare
azarovh
force-pushed
the
feat/orders_accounting
branch
from
4f53e00
to
8fe0684
Compare
| chain_config, | ||
| block_height, | ||
| command, | ||
| orders_accounting_view, |
There was a problem hiding this comment.
Why don't we use the tracker here? I'm not very comfortable with this half and half approach
There was a problem hiding this comment.
I believe tracker's purpose is to track overspends from accounts across multiple inputs. Here I need orders_view to calculate actual amounts being spend. Am I missing something?
There was a problem hiding this comment.
What prevents multiple commands from executing the same fill?
There was a problem hiding this comment.
the rule that there can only be single Command per tx
There was a problem hiding this comment.
will be reworked in a separate pr
mempool/src/error/ban_score.rs
Outdated
| Error::InvariantOrderDataNotFoundForUndo(_) => 100, | ||
| Error::InvariantOrderAskBalanceNotFoundForUndo(_) => 100, | ||
| Error::InvariantOrderAskBalanceChangedForUndo(_) => 100, | ||
| Error::InvariantOrderGiveBalanceNotFoundForUndo(_) => 100, | ||
| Error::InvariantOrderGiveBalanceChangedForUndo(_) => 100, | ||
| Error::InvariantOrderDataExistForCancelUndo(_) => 100, | ||
| Error::InvariantOrderAskBalanceExistForCancelUndo(_) => 100, | ||
| Error::InvariantOrderGiveBalanceExistForCancelUndo(_) => 100, | ||
| Error::InvariantNonzeroAskBalanceForMissingOrder(_) => 100, | ||
| Error::InvariantNonzeroGiveBalanceForMissingOrder(_) => 100, |
There was a problem hiding this comment.
Not that it's a big deal, but IMO it's better for "invariant" errors to have zero ban score, because they can't be peer's fault.
Same for chainstate/src/detail/ban_score.rs
There was a problem hiding this comment.
Maybe, not sure, it's just how it's done for every other invariant error
| #[codec(index = 11)] | ||
| AnyoneCanTake(Box<OrderData>), | ||
| } |
There was a problem hiding this comment.
Not that I insist on renaming this, but I wonder why this name was chosen. Shouldn't it be "CreateOrder" or something similar?
There was a problem hiding this comment.
The idea is that the output names describe a use case rather then specific operation performed. Like Transfer which doesn't transfer anything per se.
Introduces new
TxOutput::AnyoneCanTakewhich essentially represents a buy/sell order. This output creates an account with respective balances which can be filled by anyone partially/fully or withdrawn by an authority specified in the original output. Filling an order means creating a transaction that "burns" a currency that is being bought by the order and producing corresponding amount of other currency from the order account.Allows to exchange a token for other token, coin for any token or vice versa. Given that the exchange is arbitrary and the atoms amount not divisible price arithmetics has it's restrictions. Currently it's implemented with integers arithmetics in a way that 3 tokens sold for 100 coins one by one will cost [33; 33; 34] coins each.
Suggestions for better arithmetics as well as naming are welcome.
Api-server and wallet are stubbed in this PR.