Expose new request interface, implement first few request typings #843

FredKSchott · 2018-01-31T02:06:42Z

Now that the Flow->TypeScript migration is complete, we have support for the function type overloading that will support direct API requests (for all TypeScript projects and JavaScript projects via supported text editors like VS Code).

This PR intentionally only adds typing for a few commands for now. I want to get this reviewed first before going any further. Once this is merged the remaining requests/responses can be split up and parallelized.

The existing methods have been refactored to use the new interface internally, and the goal is still to not make any breaking changes to how they behave.

Summary

New request()/requestAll() methods (currently marked private until more baked)
New type definitions for 5 selected rippled commands:
- account_info, account_lines, account_offers, book_offers, gateway_balances
Refactored existing request methods to simplify & use those new request/response definitions
Organized & cleaned up existing type definitions (removed duplicated / overlapping types)

- src/api: add basic implementation of request/requestAll() - src/ledgers/account_info: refactor to simplify with request() - src/ledgers/balances: refactor to simplify with request() - src/ledgers/orderbook: refactor to simplify with requestAll() - src/ledgers/orders: refactor to simplify with requestAll() - src/ledgers/trustlines: refactor to simplify with requestAll()

intelliot · 2018-02-03T00:39:16Z

src/ledger/accountinfo.ts

  ledgerVersion?: number
 }

-type AccountInfoResponse = {
+type GetAccountInfoFormattedResponse = {


I see that some types were renamed, like AccountInfoResponse -> GetAccountInfoFormattedResponse, TrustlinesOptions -> GetTrustlinesOptions, Trustline -> ParsedTrustline. Could you explain a bit about how these types should be named?

Definitely, a would love some feedback from you on these as well.

The main idea behind the type renaming/reorganization is to best support the rippled API as the main source of truth. But the current AccountInfoResponse, TrustlinesOptions, and Trustline types all describe special objects created and defined specifically by ripple-lib. So, for example, now that we're adding typing info for the actual standard "account_info" response format, AccountInfoResponse is a confusing name for the custom/specialized getAccountInfo() response object. Same with the current Trustline, which is a special custom parsing of a trustline and not the standard API format for one.

What do you think? So far I haven't had followed any specific rules while renaming existing custom types that conflicted with standard formats. But happy to work out a set of rules to follow if that's a concern.

intelliot

These changes mostly look reasonable :)

The authorized property was removed which I think is a bug.

I would be interested in a proposed set of rules for naming of types.

intelliot · 2018-02-05T20:57:28Z

src/ledger/parse/account-trustline.ts

  })
  // rippled doesn't provide the counterparty's qualities
  const counterparty = removeUndefined({
    limit: trustline.limit_peer,
    ripplingDisabled: trustline.no_ripple_peer || undefined,
-    frozen: trustline.freeze_peer || undefined,
-    authorized: trustline.peer_authorized || undefined


I think we need to keep authorized. Unless there's a reason it was removed?

I didn't see peer_authorized mentioned anywhere here, and assumed it was an old removed property / always undefined. https://ripple.com/build/rippled-apis/#account-lines

Happy to put it back in if that's not true (& create issue to document)

I think it is missing from the documentation. I've opened a PR for the docs to add it in.

👍 added back

FredKSchott · 2018-02-06T19:54:20Z

@intelliot Here are the general naming guidelines I've been following so far, lmk what you think:

The goal is to re-organize and clean up existing types as we add new request/response types
Big changes to existing type definitions are avoided, there's enough going on as it is
During this transition we try to reuse existing types and avoid changing existing type names
all general types (shared across files, used in more than one file, etc.) are moved into the types/ directory.
types/commands/ stores all of the typings for rippled commands. Each file's format is roughly:

// account_offers.d.ts
export interface AccountOffersRequest { ...
export interface AccountOffersResponse { ...

// some_command.d.ts
export interface SomeCommandRequest { ...
export interface SomeCommandResponse { ...

types/objects/ stores all the typings for all shared rippled objects, like RippledAmount, Memo, Transaction types, etc.
- If an existing type name conflicts with the new standard type names (ex: AccountInfoResponse conflicts with the new "account_info" response type) we prepend the "Formatted" prefix to convey that the type has been formatted by a ripple-lib API method, and isn't necessarily a standard rippled object.

intelliot · 2018-02-06T20:04:58Z

Sounds good to me!

intelliot · 2018-02-06T20:18:41Z

Aside from authorized 🙂 , are there any other changes/cleanups that could be breaking?

FredKSchott · 2018-02-07T16:54:41Z

authorized is added back in, so nothing should be breaking/changed at this point. The only noticeable change (see tests) were that input validation for some API methods used to throw synchronously instead of rejecting the method promise. Now that we're using async functions, all throw errors reject the promise.

If a user had specific handling around function input validation, in some situations that may also need to change. It's minor, but may be worth considering when choosing the next version number.

intelliot

LGTM!

intelliot · 2018-02-07T18:54:29Z

src/common/types/objects/amounts.d.ts

+export type RippledAmount = string | Amount;
+
+/**
+ * Specification of which currency the account taking the offer would pay/receive, as an object with currency and issuer fields (omit issuer for XRP). Similar to currency amounts.


nit: this comment is long for one line (could benefit from being hard wrapped)

wilsonianb

This is great stuff @FredKSchott

wilsonianb · 2018-02-08T00:59:55Z

src/api.ts

+      // unknown string).
+      const singleResult = await (<Function>this._request)(command, repeatProps)
+      marker = singleResult.marker
+      count += singleResult[collectKey].length


Should this first check that singleResult[collectKey] exists in case the wrong collect key was provided?

Good call, do we have a precedence for rejecting if the response object is malformed?

As long as the API will always return this property for a response I think it's safe to reject if singleResult[collectKey] === undefined. But if that's not a safe assumption maybe we just stop collecting and return the responses collected so far. What do you guys think?

I'm not sure - I think either way should be fine, since this shouldn't actually happen.

Looking at some of the existing parse functions in src/ledger/parse/, we seem to be in the habit of not rejecting if the response object is malformed.
https://github.com/ripple/ripple-lib/blob/e311b74dac0a90e7a83b9126f0d64840a395e789/src/ledger/parse/payment-channel.ts#L46-L61
So I'd vote for returning the collected responses.

wilsonianb · 2018-02-08T04:50:28Z

src/common/types/objects/signers.d.ts

+export interface SignerEntry {
+  Account: string,
+  SignerWeight: number
+}


nit: missing newline here and in a few other added files

wilsonianb · 2018-02-08T05:01:24Z

src/common/types/commands/account_info.d.ts

+  transactions?: TransactionData[],
+}
+
+interface TransactionData {


It sounds like all of these are optional
https://ripple.com/build/rippled-apis/#account-info

Each object in the transactions array, if present, may contain any or all of the following fields:

wilsonianb · 2018-02-08T05:10:06Z

src/common/types/commands/account_offers.d.ts

+  offers?: AccountOffer[];
+}
+
+export interface AccountOffer {


Should also include seq: number

wilsonianb · 2018-02-08T05:12:12Z

src/common/types/commands/account_offers.d.ts

+  marker?: any;
+}
+
+export interface AccountOffersResponse {


Should also include ledger_hash?: string;
https://ripple.com/build/rippled-apis/#account-offers

wilsonianb · 2018-02-08T05:18:07Z

src/common/types/commands/book_offers.d.ts

+}
+
+export interface BookOffersResponse {
+  account: string;


~~account: string;~~

wilsonianb · 2018-02-08T05:29:14Z

src/common/types/objects/transactions.d.ts

@@ -0,0 +1,14 @@
+import {RippledAmount} from './amounts'
+
+export interface OfferCreateTransaction {


Should this include all common fields?
https://ripple.com/build/transactions/#common-fields

yea, these common fields should eventually go into some Transaction "base"/"common" type that the others can extend from. Let me add them to OfferCreateTransaction directly for now, and then clean up when we touch the rest of the Transaction types (already looking to be the most complex part of typing).

wilsonianb · 2018-02-08T06:13:08Z

src/ledger/orderbook.ts

+
+export type OrdersOptions = {
+  limit?: number,
+  ledgerVersion?: number


@intelliot I realize this isn't new, but is it possible to specify the closed or current (not validated) ledger versions? Same thing in getOffers.

Based on the docs, this is intended for specifying a historical ledger version. I'm not sure about closed/current - I'll take a closer look.

ripple-lib currently doesn't allow using not-validated ledger versions. This is something we could revisit in the future, but for now, this is intentional.

wilsonianb · 2018-02-08T17:42:32Z

src/common/types/commands/book_offers.d.ts

+
+export interface BookOffersResponse {
+  account: string;
+  offers: OfferCreateTransaction[];


https://ripple.com/build/rippled-apis/#book-offers

In addition to the standard Offer fields, the following fields may be included in members of the offers array

owner_funds?: string; taker_gets_funded?: RippledAmount; taker_pays_funded?: RippledAmount; quality?: number;

Should this instead be a different interface (OrderBookOffers?) that extends OfferCreateTransaction and includes those additional fields?

Good catch! Yea I think that's the right way to handle this

wilsonianb · 2018-02-08T18:05:54Z

src/ledger/orders.ts

+  // 2. Make Request
+  const responses = await this._requestAll('account_offers', {
+    account: address,
+    ledger_index: options.ledgerVersion || await this.getLedgerVersion(),


Can we use 'validated' instead of await this.getLedgerVersion()?

@wilsonianb My understanding is that this.getLedgerVersion() returns _ledgerVersion, which is always updated to the ledger_index of the last message. Would this be exactly similar to validated?

"validated" would be so much simpler, I'd love to make the change if we can.

I think validated would be the same. We can make the change in a later PR, though.

FredKSchott · 2018-02-09T00:20:23Z

Looks like the new types weren't linting, should be all fixed now.

@wilsonianb thanks for the feedback! Super helpful to get a second eye on those request/response type definitions. I left a few questions, PTAL when you can

intelliot

We are clamping the limit too early, preventing _requestAll from getting the correct value for countTo.

In my manual testing, it looks like limit > 400 doesn't cause any harm (I still just get 400 items back). So I doubt that clamp is actually needed.

intelliot · 2018-02-13T01:19:40Z

src/ledger/orders.ts

+  const responses = await this._requestAll('account_offers', {
+    account: address,
+    ledger_index: options.ledgerVersion || await this.getLedgerVersion(),
+    limit: utils.clamp(options.limit, 10, 400) || undefined


I think we should just pass options.limit here

intelliot · 2018-02-13T01:19:57Z

src/ledger/trustlines.ts

    account: address,
    ledger_index: ledgerVersion,
-    marker: marker,
-    limit: utils.clamp(limit, 10, 400),
+    limit: utils.clamp(options.limit, 10, 400),


Should not clamp

FredKSchott · 2018-02-13T02:13:31Z

@intelliot so I wanted to mimic the existing logic as close as possible, which meant continuing to use clamp in the existing API methods. But I totally agree it shouldn't be needed, which is why I was intentionally keeping it out of the new _requestAll method. The user will expect that request parameters passed through that function make it to the API.

Does that make sense? Happy to remove clamp from the existing logic if you don't think it's doing anything useful.

intelliot · 2018-02-13T02:53:55Z

I think the existing logic works because it clamps later in the flow, allowing more than 400 results to ultimately be returned. (The limit is 400 for an individual request, but since ripple-lib makes multiple requests, you can get more than 400 items in the end.)

intelliot · 2018-02-13T04:44:46Z

To clarify - this PR currently fails to mimic the existing logic because it doesn't replicate the behavior of utils.getRecursive. With the existing code, I am able to get multiple pages of results (by specifying limit: 401 or higher), but with this PR, I can't go further than 1 page (400 results).

intelliot · 2018-02-13T05:38:11Z

This revealed one of the (undoubtedly many) gaps in our unit test coverage. Here's a new test that catches the issue: #850

… into FredKSchott-request-interface

FredKSchott · 2018-02-14T02:01:53Z

@intelliot Ahhhh I see, so you can request > 400 total items with the original limit option, and then the existing code clamps any one request <= 400.

That's a bummer, I wanted to avoid manipulating the given request arguments as much as possible. But since we're already setting limit & marker across requestAll series, I guess that's fine for now. I'll move the logic into requestAll.

Longer term, if you said rippled is already implementing it's own clamping logic server-side, then we should defer to the server's preference and can remove the client-side clamping entirely.

intelliot · 2018-02-14T02:08:41Z

Right! I've looked at the relevant rippled code and I believe we can remove the client-side clamping entirely. I'll call out the change in the release notes when that goes out.

Out of interest, is there a reason to use the do/while instead of recursion?

FredKSchott · 2018-02-14T02:13:18Z

Just pushed the move the clamp inside requestAll(). Something I just realized: the current clamp lower bounds is 10, would you consider that a bug? If I passed limit: 1, I'd just expect to get a single result back. Can I drop that from 10 -> 1?

intelliot · 2018-02-14T02:19:59Z

I think that's intentional based on the docs for account_lines:

Must be within the inclusive range 10 to 400.

However, I don't think that's entirely accurate. Nothing bad happens if the value is beyond this range. I'll put in a PR for updating these docs and see what the rest of the team thinks.

In the current implementation, the result is truncated client-side, so if you pass limit: 1, you do get a single result back: https://github.com/ripple/ripple-lib/blob/0.18.2/src/ledger/utils.ts#L43

FredKSchott · 2018-02-14T02:21:45Z

No specific reason for a loop vs. recursion. I'll avoid saying "it's cleaner" since that's subjective, but objectively having a flat loop that doesn't involve deeply calling itself / passing around arguments is easier to follow (for humans) and statically analyze (for tooling) than a recursive function.

This is only true now that we have await, since for/do loops were impossible with promises without it and you'd end up needing to pass functions & promise handlers around anyway.

FredKSchott · 2018-02-14T02:24:38Z

@intelliot ha! I didn't even realize that. I'll add that behavior back for now as well and then we can revaluate all this again when it's time to make requestAll public.

Edit: done

intelliot

LGTM

intelliot · 2018-02-16T00:18:40Z

@wilsonianb Could you take another look?

wilsonianb

Besides the extra BookOffersResponse field and checking that singleResult[collectKey] exists, everything else LGTM

wilsonianb · 2018-02-16T04:45:20Z

src/common/types/commands/book_offers.ts

+}
+
+export interface BookOffersResponse {
+  account: string,


~~account: string;~~
https://ripple.com/build/rippled-apis/#book-offers

wilsonianb · 2018-02-16T05:17:20Z

src/api.ts

+      // unknown string).
+      const singleResult = await (<Function>this._request)(command, repeatProps)
+      marker = singleResult.marker
+      count += singleResult[collectKey].length


Looking at some of the existing parse functions in src/ledger/parse/, we seem to be in the habit of not rejecting if the response object is malformed.
https://github.com/ripple/ripple-lib/blob/e311b74dac0a90e7a83b9126f0d64840a395e789/src/ledger/parse/payment-channel.ts#L46-L61
So I'd vote for returning the collected responses.

FredKSchott · 2018-02-16T22:32:58Z

@wilsonianb thanks for the review! Addressed both 👍

FredKSchott · 2018-02-16T22:34:44Z

src/api.ts

@@ -208,7 +208,8 @@ class RippleAPI extends EventEmitter {
      lastBatchLength = singleResult[collectKey].length
      results.push(singleResult)
    } while(!!marker && count < countTo && lastBatchLength !== 0)
-    return results
+    // NOTE: We slice out any additional items over the original requested num
+    return results.slice(0, countTo)


follow up: this was reverted for now. Since results is an array of full response objects (where responses can contain multiple data objects).

wilsonianb

LGTM 👍

intelliot reviewed Feb 3, 2018

View reviewed changes

intelliot requested changes Feb 5, 2018

View reviewed changes

intelliot mentioned this pull request Feb 6, 2018

account_lines - Add authorized and peer_authorized XRPLF/xrpl-dev-portal#300

Closed

add back undocumented authorized properties

02ea8e8

standardize on Formatted prefix

1fc389e

intelliot requested a review from wilsonianb February 6, 2018 20:06

match existing request default limit of infinity

315b11d

intelliot approved these changes Feb 7, 2018

View reviewed changes

fix lint errors

e6c9260

wilsonianb reviewed Feb 8, 2018

View reviewed changes

update types, code review

0f0edbf

wilsonianb mentioned this pull request Feb 9, 2018

Fix various rippled api parameters XRPLF/xrpl-dev-portal#303

Merged

intelliot requested changes Feb 13, 2018

View reviewed changes

intelliot mentioned this pull request Feb 13, 2018

Add test of getTrustlines with multiple pages of results #850

Merged

intelliot added 5 commits February 13, 2018 15:15

Add test of getTrustlines with multiple pages of results

6969ce5

[fold] Address review comments

487de54

[fold] Return realistic number of items in test

68b334f

Merge branch 'add-trustlines-test' into FredKSchott-request-interface

a41e90a

Merge branch 'request-interface' of github.com:FredKSchott/ripple-lib…

a7c4ebe

… into FredKSchott-request-interface

clamp properly inside requestAll()

5f2e36b

slice return array to limit

920f574

intelliot approved these changes Feb 16, 2018

View reviewed changes

Merge branch 'develop' into request-interface

6b84955

wilsonianb reviewed Feb 16, 2018

View reviewed changes

cleanup, handle bad response

7af3bbb

FredKSchott commented Feb 16, 2018

View reviewed changes

wilsonianb approved these changes Feb 16, 2018

View reviewed changes

intelliot merged commit 365de6d into XRPLF:develop Feb 20, 2018

		@@ -0,0 +1,14 @@
		import {RippledAmount} from './amounts'

		export interface OfferCreateTransaction {

Expose new request interface, implement first few request typings #843

Expose new request interface, implement first few request typings #843

Conversation

FredKSchott commented Jan 31, 2018

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intelliot left a 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

FredKSchott commented Feb 6, 2018 • edited Loading

intelliot commented Feb 6, 2018

intelliot commented Feb 6, 2018

FredKSchott commented Feb 7, 2018

intelliot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonianb left a 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

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

FredKSchott commented Feb 9, 2018

intelliot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott commented Feb 13, 2018

intelliot commented Feb 13, 2018

intelliot commented Feb 13, 2018

intelliot commented Feb 13, 2018

FredKSchott commented Feb 14, 2018

intelliot commented Feb 14, 2018

FredKSchott commented Feb 14, 2018

intelliot commented Feb 14, 2018

FredKSchott commented Feb 14, 2018

FredKSchott commented Feb 14, 2018 • edited Loading

intelliot left a comment

Choose a reason for hiding this comment

intelliot commented Feb 16, 2018

wilsonianb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott commented Feb 16, 2018

Choose a reason for hiding this comment

wilsonianb left a comment

Choose a reason for hiding this comment

FredKSchott commented Feb 6, 2018 •

edited

Loading

FredKSchott commented Feb 14, 2018 •

edited

Loading