[CO-324] Accounts per-field endpoints by KtorZ · Pull Request #3210 · input-output-hk/cardano-sl

KtorZ · 2018-07-09T07:33:25Z

Description

There are needs for exchanges to retrieve only the balance of a given account. A call to `/api/v1/wallets/{walletId}/accounts/{accountId} already contains this field but is unpractical for exchanges since they usually have accounts with MANY addresses, resulting in a huge response from the API.

Linked issue

CO-324

Type of change

🐞 Bug fix (non-breaking change which fixes an issue)
🛠 New feature (non-breaking change which adds functionality)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
🔨 New or improved tests for existing code
⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

QA Steps

Screenshots (if available)

Ideally, we want the /.../account/addresses to be paginated, otherwise there's no real value in isolating it as such. Also, for now, the implementation is rather heavy and requires the full account to be retrieved from the DB. On the long-term, we could imagine fetching only the addresses from the DB and streaming the response.

This is not 'ideal' yet because we are sorting and filtering on a plain list. Instead, we could have these operation at a DB-level, but this won't probably be achieved with acid-state. Perhaps if one day we switch to SQL

There was no integration tests for any accounts endpoints :s ... We should work on that in the future as I didn't take care of it in this commit / task, but solely add a few one for the newly added endpoints

KtorZ · 2018-07-09T07:36:33Z

    IndexToQueryParam Wallet  (V1 Core.Timestamp)     = "created_at"

-    IndexToQueryParam WalletAddress (V1 Core.Address) = "id"
+    IndexToQueryParam WalletAddress (V1 Core.Address) = "address"


NOTE: Wasn't used before, so there's no breaking change here. I believe address makes more sense even though it filters on the id field of an address. As a reminder, we represent addresses as follows:

man, that does feel really weird. i agree that this is an improvement, but this is confusing.

KtorZ · 2018-07-09T07:52:10Z

+newtype AccountBalance = AccountBalance
+    { acbAmount    :: V1 Core.Coin
+    } deriving (Show, Ord, Eq, Generic)
+


This is really open to debate. This will preserve a similar structure than the one used for account. For instance, retrieving the account balance will return a JSON with:

{ "status": "success", "data": { "amount": 14 }, "metadata": "..." }

Another option would be to remove one level of nesting and put the value directly under data:

{ "status": "success", "data": 14, "metadata": "..." }

I am really open to discuss this.

What is the argument for the flattened one? I have no idea how this would affect usage in various client libraries, maybe there is some specific argument for either one. I wouldn't instinctively argue against the nested approach though.

It's a matter of perspective. I believe that the first approach allows you to re-use parser and structures defined in client code by keeping things roughly the same.
The second approach is more "correct" if we think of the request as a question, and data the answer to that question.

"What is this account's balance?"

"14"

Though, I agree, it's really a matter of preferences here. I just wanted to query your opinion(s).

Could one enforce the "keeping things roughly the same" part? When first seeing this PR I came to think about lenses and GraphQL…

I always thought about data as a container of, well, data, so it would feel unnatural to me to go from data :: Object to data :: Value, if you see what you mean. I suspect I lean towards symmetry and consistency, and as such having data always index a JSON object seems like the best approach, to me, but of course, that's just my opinion 😉

I agree that consistency and convention are important (especially without types), and right now, as far as JS is concerned, we've got a type like { data :: Object }. Deviating from that would a potential source of runtime errors.

Also, this rename the balance endpoint to '/api/v1/wallets/{id}/accounts/{ix}/amount' to reflect the actual name of the field in the Account representation.

adinapoli-iohk

Two minor quibbles , but I think the "meat" is there -- tres bien

adinapoli-iohk · 2018-07-12T10:44:55Z

+          :> Summary "Retrieve only account's addressees."
+          :> WalletRequestParams
+          :> FilterBy '[V1 Core.Address] WalletAddress
+          :> SortBy '[V1 Core.Address] WalletAddress


Does sorting on address really make sense? They are just a bunch of base58-encoded strings after all, so I'm wondering whether or not we should allow sorting, as it buys virtually nothing 😉

It allows pagination based on an indexed part of the datatype, which makes for super efficient querying.

It allows pagination based on an indexed part of the datatype, which makes for super efficient querying.

@parsonsmatt @KtorZ Isn't what FilterBy buys you already? Unless I am mistaken, note that the type-level sorting & filtering has no effect on the (already-defined) IxSet indices. Once those are defined (as in instance Indexable ...) which one you use for filter & sort (or the lack thereof) really doesn't undermine querying efficiency 😉

Imho keeping SortBy here is a mistake, as it would encourage exchanges to sort by addresses, which is an operation terribly wasteful, as there is no meaningful ordering for addresses, as the lexicographical order doesn't really help here. My 2 cents anyway, very happy to be proved wrong! 😀

In a way, pagination without ordering doesn't make much sense to me, does it? Do we even have guarantees with the acid-state db that we will yield the same elements from two successive get queries? Or is it completely non-deterministic on non-indexed items?

Though, I'd rather follow your intuition on that @adinapoli-iohk. Note that the PR has been merged in a feature-branch and not yet to develop, so there's still time to do some minor adjusments!

Haven't followed the details here but I agree that for pagination it is important to have an indexed attribute available to sort on. Not sure how much control we need to give the user over this, since as @adinapoli-iohk points out, sorting on base58 strings isn't particularly meaningful. Moreover, in the new wallet we clearly distinguish between the primary key and the rest of the data structure and don't require Ord instances for the whole type; in this case, the natural way to sort by "address" would be to sort on its key (which is basically just the pair of integers telling it how that address is derived from the root). So I think it's important that "sort by address" doesn't necessarily mean "sort lexicographically", but rather "give me some kind of pagination I can depend on" -- so that we are free to switch over the sorting to the primary key once we move to the new wallet, rather than being forced to still support this lexicographical sort.

In a way, pagination without ordering doesn't make much sense to me, does it?

Indeed, but I think we should be fine in this case, as in the current implementation we simply call IxSet.toList which, AFAIK, uses descending ordering, which is a reasonable default, I believe.

My main gripe was that, if we leave that SortBy as-it-is, we are really exposing sort_by=address[DES|ASC] to the API, which means exchanges could enforce an explicit sorting on the addresses, which, considering the amount of addresses an exchanges has, I would prefer to not pay the explicit price, if possible 😀

Am I being mental? 😛

I usually say: If users can do something, eventually they will. I wonder if we shouldn't indeed remove that sorting for now, until we have something more meaningful we can actually sort on.

If users can do something, eventually they will.

Aye, that is usually my experience -- that's why I have flagged this up 😉

NOTE: Sorting was removed on the main story branch

adinapoli-iohk · 2018-07-12T10:46:11Z

 $readAccounts
 ```

+Partial Representations


Good lad for adding documentation! 😻

adinapoli-iohk · 2018-07-12T10:47:15Z

+
+-- | Datatype wrapping balance for per-field endpoint
+newtype AccountBalance = AccountBalance
+    { acbAmount    :: V1 Core.Coin


Bikeshedding -- acbAmount or acbBalance? 🚴

I suspect the latter reads better in the output JSON -- what do you think?

Weak vote for acbBalance but I think both are fine

🙃 ... I went for acbBalance at first, because it reads better. Then I notice the fireld was actually named amount in the Account representation. Therefore, I changed for acbAmount as well for consistency.

parsonsmatt · 2018-07-12T21:20:54Z

+                        (expectExactlyAddresses [addr])
+                    , PaginationTest (Just 2) (Just 5) (filterByAddress addr) NoSorts
+                        (expectExactlyAddresses [])
+                    ]


parsonsmatt · 2018-07-12T21:24:56Z

    IndexToQueryParam Wallet  (V1 Core.Timestamp)     = "created_at"

-    IndexToQueryParam WalletAddress (V1 Core.Address) = "id"
+    IndexToQueryParam WalletAddress (V1 Core.Address) = "address"


man, that does feel really weird. i agree that this is an improvement, but this is confusing.

parsonsmatt · 2018-07-12T21:25:41Z

+          :> Summary "Retrieve only account's addressees."
+          :> WalletRequestParams
+          :> FilterBy '[V1 Core.Address] WalletAddress
+          :> SortBy '[V1 Core.Address] WalletAddress


It allows pagination based on an indexed part of the datatype, which makes for super efficient querying.

parsonsmatt · 2018-07-12T21:28:00Z

+    -> m (WalletResponse AccountAddresses)
+getAccountAddresses wId accIdx pagination filters sorts = do
+    resp <- respondWith pagination filters sorts (getAddresses <$> getAccount wId accIdx)
+    return resp { wrData = AccountAddresses . wrData $ resp }


is WalletResponse a functor/foldable? it totally should be!

parsonsmatt · 2018-07-12T21:28:31Z

+    readAddresses        = decodeUtf8 $ encodePretty $ genExample @(WalletResponse [Address])
+    readFees             = decodeUtf8 $ encodePretty $ genExample @(WalletResponse EstimatedFees)
+    readNodeInfo         = decodeUtf8 $ encodePretty $ genExample @(WalletResponse NodeInfo)
+    readTransactions     = decodeUtf8 $ encodePretty $ genExample @(WalletResponse [Transaction])


rather than large whitespace changes like this i would prefer an indentation to make the start point equal for each line. minor quibble :)

And I do agree with you!

parsonsmatt · 2018-07-12T21:29:13Z

+
+-- | Datatype wrapping balance for per-field endpoint
+newtype AccountBalance = AccountBalance
+    { acbAmount    :: V1 Core.Coin


Weak vote for acbBalance but I think both are fine

parsonsmatt · 2018-07-12T21:30:07Z


+instance Arbitrary AccountAddresses where
+    arbitrary =
+        AccountAddresses <$> arbitrary


with deriving strategies, this is deriving newtype (Arbitrary) 😄

I am always (without any good reasons) afraid of deriving newtypes automagically :p ...

…endpoints [CO-324] Accounts per-field endpoints

…hk/KtorZ/CO-324/per-field-endpoints [CO-324] Accounts per-field endpoints

KtorZ added 3 commits July 6, 2018 16:14

[CO-324] Introduce pagination for getAccountAddresses

930c8c0

This is not 'ideal' yet because we are sorting and filtering on a plain list. Instead, we could have these operation at a DB-level, but this won't probably be achieved with acid-state. Perhaps if one day we switch to SQL

[CO-324] Extend integration test suites for account per-field endpoint

323d0fc

There was no integration tests for any accounts endpoints :s ... We should work on that in the future as I didn't take care of it in this commit / task, but solely add a few one for the newly added endpoints

KtorZ self-assigned this Jul 9, 2018

KtorZ requested a review from parsonsmatt July 9, 2018 07:33

KtorZ requested review from adinapoli-iohk and denisshevchenko as code owners July 9, 2018 07:33

KtorZ commented Jul 9, 2018

View reviewed changes

[CO-324] Write about partial account representations in API doc

83c865e

Also, this rename the balance endpoint to '/api/v1/wallets/{id}/accounts/{ix}/amount' to reflect the actual name of the field in the Account representation.

KtorZ requested a review from Anviking July 9, 2018 09:11

adinapoli-iohk approved these changes Jul 12, 2018

View reviewed changes

parsonsmatt approved these changes Jul 12, 2018

View reviewed changes

KtorZ merged commit 2027e1e into Squad1/CO-325/api-v1-improvements Jul 16, 2018

KtorZ added a commit that referenced this pull request Aug 8, 2018

Merge pull request #3210 from input-output-hk/KtorZ/CO-324/per-field-…

abe9f5a

…endpoints [CO-324] Accounts per-field endpoints

KtorZ added a commit that referenced this pull request Aug 9, 2018

Merge pull request #3210 from input-output-hk/KtorZ/CO-324/per-field-…

3e2a136

…endpoints [CO-324] Accounts per-field endpoints

KtorZ deleted the KtorZ/CO-324/per-field-endpoints branch August 16, 2018 14:53

KtorZ added a commit that referenced this pull request Nov 9, 2018

Merge pull request #3210 from input-output-hk/KtorZ/CO-324/per-field-…

c84f51b

…endpoints [CO-324] Accounts per-field endpoints

KtorZ added a commit to input-output-hk/cardano-wallet-legacy that referenced this pull request Nov 9, 2018

Merge pull request input-output-hk/cardano-sl#3210 from input-output-…

6239716

…hk/KtorZ/CO-324/per-field-endpoints [CO-324] Accounts per-field endpoints

Conversation

KtorZ commented Jul 9, 2018

Description

Linked issue

Type of change

Developer checklist

Testing checklist

QA Steps

Screenshots (if available)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KtorZ Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adinapoli-iohk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adinapoli-iohk Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

KtorZ Jul 9, 2018 •

edited

Loading

adinapoli-iohk Jul 16, 2018 •

edited

Loading