Conversation
OBorce
force-pushed
the
feature/api-server-mempool-txs
branch
2 times, most recently
from
554e4e2 to
b00d078
Compare
OBorce
force-pushed
the
fix/api-server-balance-overflow
branch
from
e19ea86 to
184291a
Compare
OBorce
force-pushed
the
feature/api-server-mempool-txs
branch
from
b00d078 to
8a9d0ce
Compare
OBorce
force-pushed
the
feature/api-server-mempool-txs
branch
from
8a9d0ce to
6496fe8
Compare
OBorce
force-pushed
the
feature/api-server-mempool-txs
branch
from
6496fe8 to
b916d76
Compare
OBorce
force-pushed
the
feature/api-server-mempool-txs
branch
from
b916d76 to
1f9d564
Compare
ImplOfAnImpl
left a comment
ImplOfAnImpl
left a comment
There was a problem hiding this comment.
CURRENT_STORAGE_VERSION wasn't updated - is this intended?
| - API Server: | ||
| - new endpoints for addresses and transactions from the mempool: | ||
| - `/mempool/transaction` | ||
| - `/mempool/transaction/:id` | ||
| - `/mempool/transaction/:id/output/:idx` | ||
| - `/mempool/address/:address` | ||
| - `/mempool/address/:address/all-utxos` | ||
|
|
There was a problem hiding this comment.
API server has a separate changelog - api-server/CHANGELOG.md
| transaction_id: Id<Transaction>, | ||
| transaction: &TransactionInfo, | ||
| ) -> Result<(), ApiServerStorageError> { | ||
| // Because of ON DELETE CASCADE on other tables, we insert the transaction first. |
There was a problem hiding this comment.
Outdated comment? This function only inserts the transaction and does nothing else, so "we insert the transaction first" doesn't seem to make much sense.
| pub enum BlockchainStateError { | ||
| #[error("Unexpected storage error: {0}")] | ||
| StorageError(#[from] ApiServerStorageError), | ||
| #[error("Token decimals not found for token id: {0}")] |
There was a problem hiding this comment.
Btw, the ids are printed in contracted form by default (due to how Display is implemented for fixed_hash types), which is not very useful, especially in error messages. You can force the full form by using {0:x}.
| &self, | ||
| tx_id: Id<Transaction>, | ||
| ) -> Result<Option<SignedTransaction>, Self::Error>; | ||
| async fn mempool_get_transactions(&self) -> Result<Vec<SignedTransaction>, Self::Error>; |
There was a problem hiding this comment.
Btw, the doc comment for MempoolRpc::get_all_transactions still says "This function is mostly used for testing purposes", which is no longer true.
| address: &str, | ||
| ) -> Result<BTreeMap<CoinOrTokenId, AmountWithDecimals>, ApiServerStorageError>; | ||
|
|
||
| async fn get_utxo_mempool_fallback( |
There was a problem hiding this comment.
get_mempool_utxo_with_fallback?
Also,
- As I can see, a bunch of other functions also fall back to non-mempool ones if the info is not found in the mempool - get_mempool_address_balance, get_mempool_locked_address_balance, get_mempool_token_num_decimals, get_mempool_order. Why aren't they named in the same way?
- Maybe add doc comments describing what the functions are supposed to do exactly, especially the "fallback" ones?
| ) | ||
| .await; | ||
| } else { | ||
| increase_mempool_locked_address_balance( |
There was a problem hiding this comment.
I see that you increase locked balances in the mempool but never decrease them.
Mempool has some timelock tolerance defined by constants FUTURE_TIMELOCK_TOLERANCE/FUTURE_TIMELOCK_TOLERANCE_BLOCKS. So it should be possible to have e.g. a tx with LockThenTransfer output for 2 blocks and then a tx that consumes that output both in the mempool at the same time.
Probably you should decrease locked balances if a LockThenTransfer utxo is spent by a mempool tx.
| let (best_block_height, _, best_block_timestamp) = self.best_block().await?; | ||
| let next_block_height = best_block_height.next_height(); | ||
|
|
||
| let mut db_tx = self.storage.transaction_rw().await.expect("Unable to connect to database"); |
There was a problem hiding this comment.
Btw why do you panic on storage errors everywhere, when this function already returns an error type constructible from ApiServerStorageError?
| } | ||
|
|
||
| fn get_tx_output_destination(txo: &TxOutput) -> Option<&Destination> { | ||
| fn get_tx_output_destination(txo: &TxOutput) -> Vec<&Destination> { |
| let input_tasks: FuturesOrdered<_> = | ||
| tx.inputs().iter().map(|input| fetch_mempool_utxo(input, db_tx)).collect(); | ||
| let input_utxos: Vec<Option<Utxo>> = input_tasks.try_collect().await?; |
There was a problem hiding this comment.
Again, this will fail if the utxo belongs to a mempool tx that hasn't been handled yet.
| .route( | ||
| "/mempool/transaction/:id/output/:idx", | ||
| get(mempool_transaction_output), | ||
| ); |
There was a problem hiding this comment.
So this accepts both txs in mempool and already mined ones. Why not add an option to the existing /transaction/:id/output/:idx so that it would consider mempool txs as well?
2 participants
API Server now scans the mempool after finishing syncing to the latest block.
New address and transaction endpoints for things from the mempool.