Add tx order number and pagination #1930
Conversation
api-server/api-server-common/src/storage/impls/postgres/queries.rs
Outdated
Show resolved
Hide resolved
| }) | ||
| .collect() | ||
| } | ||
|
|
||
| pub async fn get_transactions_with_block_by_order_number( |
There was a problem hiding this comment.
This function is confusing - it's unclear that it'll return transactions before the specified order number.
Plz at least rename it, so that the name reflects what it does.
(But I'd also suggest making it return txs starting from the specified order number and making the callers do the adjustment if needed)
There was a problem hiding this comment.
renamed to include before. I am not sure starting will be good, as the main use case is fetching latest txs so you are starting from the last towards 0.
api-server/api-server-common/src/storage/impls/postgres/queries.rs
Outdated
Show resolved
Hide resolved
api-server/api-server-common/src/storage/impls/in_memory/mod.rs
Outdated
Show resolved
Hide resolved
| @@ -92,10 +93,22 @@ impl ApiServerStorageRead for ApiServerInMemoryStorageTransactionalRo<'_> { | |||
| &self, | |||
| len: u32, | |||
| offset: u32, | |||
| ) -> Result<Vec<(BlockAuxData, TransactionInfo)>, ApiServerStorageError> { | |||
| ) -> Result<Vec<TransactionWithBlockInfo>, ApiServerStorageError> { | |||
There was a problem hiding this comment.
Perhaps the function should now be called "get_transactions_with_block_info"?
Same for get_transactions_with_block_by_order_number.
| @@ -117,6 +117,7 @@ impl<S: ApiServerStorage + Send + Sync> LocalBlockchainState for BlockchainState | |||
| disconnect_tables_above_height(&mut db_tx, common_block_height) | |||
| .await | |||
| .expect("Unable to disconnect tables"); | |||
| let mut next_order_number = db_tx.get_last_transaction_order_number().await? + 1; | |||
There was a problem hiding this comment.
Btw, this "order number" naming is confusing IMO, it looks like it's related to orders.
I'd suggest naming it differently (everywhere). Some alternatives:
- "global order number" - no longer feels like it's related to orders,
- "global ordering number",
- "global index".
There was a problem hiding this comment.
renamed to tx global index
api-server/web-server/src/api/v2.rs
Outdated
| @@ -448,27 +448,59 @@ pub async fn transactions<T: ApiServerStorage>( | |||
| Query(params): Query<BTreeMap<String, String>>, | |||
| State(state): State<ApiServerWebServerState<Arc<T>, Arc<impl TxSubmitClient>>>, | |||
| ) -> Result<impl IntoResponse, ApiServerWebServerError> { | |||
| const BEFORE_ORDER_NUMBER: &str = "before_order_number"; | |||
There was a problem hiding this comment.
Why is this needed? Is this something you've discussed with Sergey?
IMO it'd be nicer if the endpoint remained the same, but with "offset" now referring to the global tx order.
There was a problem hiding this comment.
offset is increasing and the order_number now global index is decreasing, and we don't want to make breaking changes so we keep both.
OBorce
force-pushed
the
feature/api-server-tx-order-num
branch
from
e0256e6 to
909249f
Compare
Add order number to transactions based on the order inside the blocks.
Add new query parameter to the transactions endpoint to control the pagination based on the order number instead of an offset to get more stable pagination in the case of new txs being inserted.