Transaction pagination + full query arguments #666

yeastplume · 2022-11-22T13:35:16Z

Addition of new structs in libwallet internal types, RetrieveTxQueryArgs, RetrieveTxQuerySortOrder, RetrieveTxQuerySortField
Update internal retrieve_txs API function to optionally accept a RetrieveTxQueryArgs struct
Implementation of filtering via apply_advanced_tx_list_filtering fn
Modify retrieve_tx implementation to use advanced filfering function if optional query args are supplied
Addition of unit tests to exercise all new query functionality and sorting
Add new JSON API Function query_txs, (note it was necessary to provide a separate function to avoid breaking existing json calls to retrieve_txs
Documentation and Doctesting additions for JSON API
Note that advanced query functionality is API only and not accessible from the command-line

While creating https://github.com/mimblewimble/grin-gui, it's become apparent that we need to implement much more granular transaction querying to better support pagination, sorting, etc. Instead of just implementing basic pagination, I'm taking the opportunity to add a reasonable number of advanced query options via a new query structure.

Details posted here for review and comment, I think this should cover most needs but happy to hear from anyone with suggestions or anything else that should be included in the query arguments:

pub struct RetrieveTxQueryArgs {
	/// Retrieve transactions with an id higher than or equal to the given
	/// If None, consider items from the first transaction and later
	pub min_id: Option<u32>,
	/// Retrieve tranactions with an id less than or equal to the given
	/// If None, consider items from the last transaction and earlier
	pub max_id: Option<u32>,
	/// The maximum number of transactions to return
	/// if both `before_id` and `after_id` are supplied, this will apply
	/// to the before and earlier set
	pub limit: Option<u32>,
	/// whether to exclude cancelled transactions in the returned set
	pub exclude_cancelled: Option<bool>,
	/// whether to only consider outstanding transactions
	pub include_outstanding_only: Option<bool>,
	/// whether to only consider confirmed-only transactions
	pub include_confirmed_only: Option<bool>,
	/// whether to only consider sent transactions
	pub include_sent_only: Option<bool>,
	/// whether to only consider received transactions
	pub include_received_only: Option<bool>,
	/// whether to only consider coinbase transactions
	pub include_coinbase_only: Option<bool>,
	/// whether to only consider reverted transactions
	pub include_reverted_only: Option<bool>,
	/// lower bound on the total amount (amount_credited - amount_debited), inclusive
	#[serde(with = "secp_ser::opt_string_or_u64")]
	#[serde(default)]
	pub min_amount: Option<u64>,
	/// higher bound on the total amount (amount_credited - amount_debited), inclusive
	#[serde(with = "secp_ser::opt_string_or_u64")]
	#[serde(default)]
	pub max_amount: Option<u64>,
	/// lower bound on the creation timestamp, inclusive
	pub min_creation_timestamp: Option<DateTime<Utc>>,
	/// higher bound on on the creation timestamp, inclusive
	pub max_creation_timestamp: Option<DateTime<Utc>>,
	/// lower bound on the confirmation timestamp, inclusive
	pub min_confirmed_timestamp: Option<DateTime<Utc>>,
	/// higher bound on the confirmation timestamp, inclusive
	pub max_confirmed_timestamp: Option<DateTime<Utc>>,
	/// Field within the tranasction list on which to sort
	/// defaults to ID if not present
	pub sort_field: Option<RetrieveTxQuerySortField>,
	/// Sort order, defaults to ASC if not present (earliest is first)
	pub sort_order: Option<RetrieveTxQuerySortOrder>,
}

Sort fields (and sort order) are:

/// Sort tx retrieval order
#[derive(Clone, Serialize, Deserialize)]
pub enum RetrieveTxQuerySortOrder {
	/// Ascending
	Asc,
	/// Descending
	Desc,
}

/// Valid sort fields for a transaction list retrieval query
#[derive(Clone, Serialize, Deserialize)]
pub enum RetrieveTxQuerySortField {
	/// Transaction Id
	Id,
	/// Creation Timestamp
	CreationTimestamp,
	/// Confirmation Timestamp
	ConfirmationTimestamp,
	/// TotalAmount (AmountCredited-AmountDebited)
	TotalAmount,
	/// Amount Credited
	AmountCredited,
	/// Amount Debited
	AmountDebited,
}

api calls throughout

…allet test to be filled out

…, inclusion of bigint, addition of unit tests to exercise filtering

…functions

yeastplume · 2022-11-29T13:49:04Z

This is now ready for review, changed original comments to reflect changes

phyro · 2022-12-01T10:42:13Z

libwallet/src/api_impl/types.rs

+	/// to the before and earlier set
+	pub limit: Option<u32>,
+	/// whether to exclude cancelled transactions in the returned set
+	pub exclude_cancelled: Option<bool>,


Is there a specific reason why we wrap all bools into options? Would removing these simplify the logical branching in the filtering code?

if they weren't options, you'd need to specify each and every one when passing in the struct from JSON, this allows json callers to leave them out

controller/tests/tx_list_filter.rs

phyro · 2022-12-01T11:33:36Z

libwallet/src/api_impl/types.rs

+	/// lower bound on the total amount (amount_credited - amount_debited), inclusive
+	#[serde(with = "secp_ser::opt_string_or_u64")]
+	#[serde(default)]
+	pub min_amount_inc: Option<u64>,


probably makes sense to remove the _inc here as well since the max_amount doesn't have it either

missed that, thank you

phyro · 2022-12-01T11:46:29Z

libwallet/src/internal/updater.rs

-		.collect();
-	txs.sort_by_key(|tx| tx.creation_ts);
+	let mut txs;
+	// Adding in new tranasction list query logic. If `tx_id` or `tx_slate_id`


nitpick: typo tranasction

phyro · 2022-12-01T11:49:13Z

libwallet/src/internal/updater.rs

+	let mut txs;
+	// Adding in new tranasction list query logic. If `tx_id` or `tx_slate_id`
+	// is provided, then `query_args` is ignored and old logic is followed.
+	if query_args.is_some() && tx_id.is_none() && tx_slate_id.is_none() {


FUTURE: Maybe we could consider returning an error if query_args is provided as well as a tx_id or slate_id i.e. you might filter to exclude cancelled txs, but the tx_id is a cancelled tx and will thus be returned.

phyro · 2022-12-01T12:48:19Z

libwallet/src/internal/updater.rs

+
+	let mut return_txs: Vec<TxLogEntry> = txs_iter.collect();
+
+	// Now apply requested sorting


I think we should apply sorting before we limit otherwise we end up with what I would consider an incorrect logic i.e. we sort things by credited amount, but only after we have cut off most of the txs meaning we won't get the most credited txs.

Not sure I'm with you here, sorting the entire data set on every query is very inefficient and should be avoided, I think. Can you give an example of where you'd get incorrect logic here? If you're interested in credited amount, you filter for it then optionally sort the results.

I agree this is slower because the sorting takes place on a larger sequence. Here's an example of what I had in mind. Let's say I have 60 transactions in my transaction log. The first 20 entries have amount credited +1, next 20 entries have amount credited +20 and the last 20 entries have amount credited +10.

I make the following call

api.query_txs(limit=10, sort_order="Desc", sort_field="AmountCredited")

I would assume this returns the top10 transactions with the largest credited amount.

My understanding is that the limit will be applied on a sequence of all tx logs in this case and will take only the last 10 from the sequence (likely take the last 10 transactions) and then sort these. The result returned will be 10 transactions with amount credited +10. The user would probably expect to get 10 transactions with +20 instead.

Okay, I'm with you now. Good catch, I've updated the logic to apply limiting after sorting.

phyro

Great job! I played a bit with the python api call wrapper by adding the following:

    def query_txs(
            self, min_id=None, max_id=None, limit=None, exclude_cancelled=None, include_outstanding_only=None,
            include_confirmed_only=None, include_sent_only=None, include_received_only=None,
            include_coinbase_only=None, include_reverted_only=None,min_amount=None, max_amount=None,
            min_creation_ts=None, max_creation_ts=None, min_confirmed_ts=None, max_confirmed_ts=None,
            sort_field=None, sort_order=None, refresh=True
    ):
        params = {
            'token': self.token,
            'refresh_from_node': refresh,
            'query': {
                "min_id": min_id,
                "max_id": max_id,
                "limit": limit,
                "exclude_cancelled": exclude_cancelled,
                "include_outstanding_only": include_outstanding_only,
                "include_confirmed_only": include_confirmed_only,
                "include_sent_only": include_sent_only,
                "include_received_only": include_received_only,
                "include_coinbase_only": include_coinbase_only,
                "include_reverted_only": include_reverted_only,
                "min_amount_inc": min_amount,
                "max_amount": max_amount,
                "min_creation_timestamp": min_creation_ts,
                "max_creation_timestamp": max_creation_ts,
                "min_confirmed_timestamp": min_confirmed_ts,
                "max_confirmed_timestamp": max_confirmed_ts,
                "sort_field": sort_field,
                "sort_order": sort_order,
            },
        }
        resp = self.post_encrypted('query_txs', params)
        if refresh and not resp["result"]["Ok"][0]:
            # We requested refresh but data was not successfully refreshed
            raise WalletError("query_txs", params, None, "Failed to refresh data from the node")
        return resp["result"]["Ok"][1]


print([x["id"] for x in wallet_api.query_txs(min_id=100, max_id=200, limit=10, sort_order="Desc", sort_field="AmountCredited")])

I didn't manage to test all the fields, but from what I found, I'd say only the last comment is more relevant.

phyro

Everything has been addressed 👍

yeastplume added 8 commits November 22, 2022 12:04

add query pagination options struct

746c183

update transaction

cd3203b

api calls throughout

added transaction filtering implementation, addition of internal libw…

806aa98

…allet test to be filled out

rename filter test

d29b642

addition of tx statuses to sort fields, fixes to total amount queries…

13b1fc8

…, inclusion of bigint, addition of unit tests to exercise filtering

add doctest exercising advanced query JSON API

1188f32

split JSON api functions into existing retrieve_tx and new query_txs …

17edec2

…functions

revert test change

ae71450

yeastplume changed the title ~~[WIP] Transaction pagination + full query arguments~~ Transaction pagination + full query arguments Nov 29, 2022

yeastplume requested a review from phyro November 29, 2022 13:47

remove _inc from field names

b1beca4

phyro reviewed Dec 1, 2022

View reviewed changes

controller/tests/tx_list_filter.rs Show resolved Hide resolved

phyro reviewed Dec 1, 2022

View reviewed changes

phyro requested changes Dec 1, 2022

View reviewed changes

yeastplume added 2 commits December 5, 2022 11:14

more tweaks based on review

2a59902

move result count limiting to after sorting

319b9ca

phyro approved these changes Dec 6, 2022

View reviewed changes

phyro merged commit 270321c into mimblewimble:master Dec 6, 2022

yeastplume mentioned this pull request Dec 6, 2022

[WIP] Home / Transaction Screen Updates mimblewimble/grin-gui#26

Merged

yeastplume mentioned this pull request Dec 20, 2022

the get_stored_tx method from OwnerAPI gives "error code -32099 because: IO" #665

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transaction pagination + full query arguments #666

Transaction pagination + full query arguments #666

yeastplume commented Nov 22, 2022 •

edited

Loading

yeastplume commented Nov 29, 2022

phyro Dec 1, 2022 •

edited

Loading

yeastplume Dec 1, 2022

phyro Dec 1, 2022

yeastplume Dec 5, 2022

phyro Dec 1, 2022

yeastplume Dec 5, 2022

phyro Dec 1, 2022

phyro Dec 1, 2022

yeastplume Dec 5, 2022

phyro Dec 5, 2022

yeastplume Dec 6, 2022

phyro left a comment

phyro left a comment


		let mut return_txs: Vec<TxLogEntry> = txs_iter.collect();

		// Now apply requested sorting

Transaction pagination + full query arguments #666

Transaction pagination + full query arguments #666

Conversation

yeastplume commented Nov 22, 2022 • edited Loading

yeastplume commented Nov 29, 2022

phyro Dec 1, 2022 • edited Loading

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

phyro left a comment

Choose a reason for hiding this comment

phyro left a comment

Choose a reason for hiding this comment

yeastplume commented Nov 22, 2022 •

edited

Loading

phyro Dec 1, 2022 •

edited

Loading