Cardano-wallet: Implement `transaction list` command in CLI

[KtorZ]

Context

Follow-up for #466 and #465. We do want to provide this capability in the CLI too, in a very similar fashion to what we currently do with other commands.

Decision

This ticket can in practice starts after #465 but can only be completed once #466 is done. The CLI will support filtering through two extra options --start & --end both optional and accepting a date in ISO-8601 UTC format but no wildcard (the absence of option has the semantic of a wildcard).

The transaction history should be displayed in JSON format, as we do for addresses.

Acceptance Criteria

1. transaction list command _must_ be implemented in the CLI
2. transaction list command _must_ provide an option to specify an upper-bound for filtering.
3. transaction list command _must_ provide an option to specify a lower-bound for filtering.
4. transaction list command _must_ provide an option to specify a sort order, either ascending or descending.
5. The option in 2. _should_ be named --start.
6. The option in 3. _should_ be named --end.
7. The option in 4. _should_ be named --order.
   

   8. The absence of upper or lower bound _should_ have the same semantic value as the wildcard *.

Development Plan

• [x] add a transaction list command to the CLI.
• [x] add a CLI unit test to demonstrate that transaction list --help displays the expected help text.
• [x] add a CLI integration test to confirm that transaction list returns [] for a brand new wallet, regardless of what values are provided for --start or --end (as expected).
• [ ] add CLI integration tests to confirm that the full set of transactions is returned when neither --start nor --end are specified.
• [ ] add CLI integration tests to confirm that appropriate subsets of transactions are returned when either --start or --end are specified (or both).

PR

| Number | Base |
| --- | --- |
| #526 | master |
| #532 | master |

QA

[KtorZ]

• [x] review must/should semantic above for options

[jonathanknowles]

@KtorZ @piotr-iohk

Regarding:

• The option in 2. _should_ be named --before.
• The option in 3. _should_ be named --after.

I think I can see a problem with using these _particular_ parameter names.

Problem

Currently, specifying --after t1 and --before t2 will eventually cause the construction of an Iso8601Range t1 t2 value, under the hood. This value will be communicated through the API to the back end.

There are two cases:

• If t1 < t2, then the back end will return results in ascending order, from t1 to t2. (So far so good.)

• If t1 > t2, then the back end will return results in descending order, from t1 to t2, as you might expect. However, the problem here is that we're no longer returning results that occur after t1 and before t2. This contradicts the names of the CLI arguments.

Therefore, I'd like to propose the following options:

Option 1: Rename the command line arguments

We could rename the arguments to --start and --end (or --from and --to).

Examples:

$ cardano-wallet transaction list <wallet-id> --from 2008 --to 2009
(results from 2008 to 2009 in ascending order)

$ cardano-wallet transaction list <wallet-id> --from 2009 --to 2008
(results from 2009 to 2008 in descending order)



md5-990d1d3b051c5aa0c87d8908d50a7001



$ cardano-wallet transaction list <wallet-id> --from 2008
(results from 2008 in ascending order)



md5-990d1d3b051c5aa0c87d8908d50a7001



$ cardano-wallet transaction list <wallet-id> --to 2009
(results until 2009 in ascending order)

Option 2: Make the order explicit in the CLI

Require that t1 < t2 for --after t1 --before t2. If not, then make the CLI return an error.

This option has the drawback that it would restrict the CLI to only ever returning results in ascending order.

However, we can overcome this restriction by adding a third argument: --order, which can take the following values:

• ascending
• descending

Question

Out of these options, which would people prefer?

[piotr-iohk]

Yes, definitely --after and --before are not fortunate names if that's the case. (I was actually thinking of a test case.. what if --after is bigger than --before and wouldn't expect such behavior :sweat_smile:) I'd be for --from and --to [or maybe to be consistent with the [api](https://input-output-hk.github.io/cardano-wallet/api/edge/#tag/Transactions) (--range-start and --range-end)?]

[KtorZ]

Good point. No particular preference between from / to and start / end (perhaps from / to reads better actually... :thinking: )

[rvl]

The range header shouldn't be overloaded for specifying sort direction (see my comment in #466). The names "before" and "after" would be fine if it weren't for this unexpected behaviour.

[jonathanknowles]

@rvl wrote:

The range header shouldn't be overloaded for specifying sort direction (see my comment in #466). The names "before" and "after" would be fine if it weren't for this unexpected behaviour.

From the above link:

Also, there is no way to set the sort order for an (half-) open range. I think we should use a query parameter to provide sort field and direction.

I think this is a good point. But fixing this would require us to change the Iso8601Range API type, as well as the API specification.

@KtorZ what are your thoughts on this?

[KtorZ]

We could indeed make the ordering explicit by extending the format with an [;order=asc|desc]. If we go that way, we could add more meta-parameters like limit, following a similar syntax: [;limit=<integer>].

A few examples:

• Range: inserted-at 20190227T160329Z-*; order=asc; limit=10
• Range: inserted-at 20190227T160329Z-*; limit=10; order=desc
• Range: inserted-at 20190227T160329Z-20200227T160329Z; limit=100
• Range: inserted-at 20190227T160329Z-20200227T160329Z; order=asc

[KtorZ]

What do you think?

@rvl @jonathanknowles

[jonathanknowles]

What do you think?
@rvl @jonathanknowles

Hi @KtorZ

@rvl and I just had a discussion about this. We feel it might be better to encode these arguments as query parameters. For example:

/wallets/<wallet-id>/transactions
/wallets/<wallet-id>/transactions?start=<time>
/wallets/<wallet-id>/transactions?end=<time>
/wallets/<wallet-id>/transactions?start=<time>&end=<time>
/wallets/<wallet-id>/transactions?start=<time>&end=<time>&order=ascending
/wallets/<wallet-id>/transactions?start=<time>&end=<time>&order=descending&limit=10

Advantages:

1. We don't need to document and validate a special format.
2. Dedicated handling of separators (-, and so on) is replaced with standardized parsing of query parameters (which is handled for us by Servant).
3. Because we're not relying on the - separator, we can support all ISO 8601-formatted times, rather than just the basic format. We already have a tested parser that does the right thing.
4. There's now a straightforward mapping from CLI arguments to query parameters.
5. There's now only one way to encode the infinite range: simply don't supply either start or end. (Before, there were two ways to encode this range.)

@KtorZ Would you be okay with this scheme? I can make the relevant changes, if you're happy with us to go ahead.

[KtorZ]

Well, to be honest, I think query parameters shouldn't be used to convey meta-information like pagination in a REST API; This is abusing the HTTP specification which already provides mechanism for such things (like request headers, in particular Range, Content-Range, Next-Range). I find query parameters rather inelegant for that purpose, and usually greatly abused with all sort of symbols that "happened" to be valid URI characters (>, =>, =<, ! etc ...). We tend to forget that APIs are designed to be used by programs, not by humans directly (and I believe, this is why we're usually inclined to bloat query parameters with request's metadata!)

For the record, I am quite fond of what Heroku's engineering team did with their API regarding pagination and versionning:

• https://devcenter.heroku.com/articles/platform-api-reference#ranges.
• https://devcenter.heroku.com/articles/platform-api-reference#schema

---

Having said that, if both of you guys thinks that it'd be better to go for query parameters instead, I won't fight too much over it and will refrain my _"principled approach"_ :grimacing: ... A couple of things to keep in mind though:

1. Every value should be URI-encoded and URI-decoded (so spaces becomes %20 etc ...)
2. We must not abuse the URI format by using symbols in non-conventional way. (e.g. https://cardanodocs.com/technical/wallet/api/v1/#section/Filtering-and-Sorting :man_shrugging:)
3. We will not support anything like skip or offset which leads to more problems than benefits in practices and, incidentally makes the pagination API non-orthogonal.

Let's do this @rvl @jonathanknowles

[jonathanknowles]

Hi @KtorZ

This is abusing the HTTP specification which already provides mechanism for such things (like request headers, in particular Range, Content-Range, Next-Range).

Thanks for sharing your perspective. I actually wasn't aware of this point of view.

For the record, I am quite fond of what Heroku's engineering team did with their API regarding pagination and versioning

Also, thanks for sharing this. I'll take a look.

As for:

Every value should be URI-encoded and URI-decoded (so spaces becomes %20 etc ...)

I was under the impression that the ToHttpApiData class handles this automatically, provided that we use toUrlPiece and not toEncodedUrlPiece (in which case we must handle encoding ourselves).

To check this assumption, I coded up the start parameter as a query parameter, and ran a test through the CLI, with an ISO 8601 extended date (so that - or : characters would be part of the value). The log reports:

[iohk.cardano-wallet.serve.api.request-4:Info:ThreadId 118] [2019-07-25 09:13:26.23 UTC] [GET] /v2/wallets/5d719bd45092695570b0ac3c0953bdc84985b85e/transactions?start=2018-08-08T08%3A08%3A08Z

So it seems that URI encoding is handled already, unless I have missed something obvious?

We will not support anything like skip or offset which leads to more problems than benefits in practices and, incidentally makes the pagination API non-orthogonal.

I'll make sure to not include anything like this.

[Anviking]

Hello all! I just wanted to say that I'm a big fan of the limit parameter in particular.

If you implement an iOS UITableView with an listTransactions and infinite scroll, you're interested in loading an appropriate chunk of transactions as the user is getting close to the bottom. limit would give you fine control over the size of the response. It seems to me that only being able to specify a date-range could get you either 10000 transactions or 0, depending on the distribution of txs across time. Or having to blindly trust the Next-Range without any control.