search

XRPLF / xrpl.js

A JavaScript/TypeScript API for interacting with the XRP Ledger in Node.js and the browser
https://xrpl.org/
1.2k stars 512 forks source link

Proposal: Simplified Requests, Flexible Responses #812

Closed FredKSchott closed 4 years ago

FredKSchott commented 6 years ago

Following up from #799:

@intelliot: @FredKSchott I'm in favor of returning all API data to the consumer, as you said. Historically, RippleAPI was designed independent of rippled. However, moving forward, we'd like to unify their APIs and make them as similar as possible. Please feel free to take a stab at this!

So I've experimented a bit and have a proposal. Even with 100% proper typing, I'm nervous to make any big changes to the existing API methods and the interfaces/data that they already return. There are a lot of interconnected parts internally, so I'm not just worried about breaking external consumers with a lot of small, hard to detect/message changes.

This proposal aims to reduce the complexity & maintainance costs of having a complete secondary ripple-lib API on top of the standard Rippled API:

Proposal: Simplified API Request Interface

All custom API methods (getTransactions(), getTrustlines(), etc) are deprecated in favor of just two new general-purpose API methods:

class RippleAPI:

  // Merge {...data, command} into a single request and send to the server.
  // Return the response object directly from the server.
  async request(command: string, data?: Object): Response;

  // Like request(), but requests multiple pages until the `limit.max` number
  // of `limit.collect` objects are returned from the response.
  async requestAll(command: string, data?: Object, limit?: {max: number, collect: string}): Response[];

Examples

- return api.getAccountInfo('r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59');
  return api.request('account_info', {account: 'r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59'});

- return api.getOrders('r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59');
  return api.request('account_offers', {
    account: 'r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59',
    ledger_index: 'validated',
  });

- return api.getOrderbook('r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59', {
-   "base": {"currency": "XRP"},
-   "counter": {"currency": "USD", "issuer": "rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B"}
- });
  return api.requestAll('book_offers', {
    taker: 'r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59'
    taker_gets: {'currency': 'XRP'},
    taker_pays: {'currency': 'USD', 'issuer': 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B'},
  }, {max: 100, collect: 'offers'});

Benefits

Support for Request Validation

Request validation can be easily moved into the general request()/requestAll() method:

async request(command: string, data: Object) {
  validateRequest(command, data); // Throw/warn if `data` is invalid for `command`
  // make request...
}

Support for Typing

Overloading the request function signature would allow powerful typing support for request()/requestAll() arguments and response format:

class RippleAPI:
  async request(command: 'account_info', data: AccountInfoRequest): AccountInfoResponse;
  async request(command: 'account_offers', data: AccountOffersRequest): AccountOffersResponse;
  async request(command: 'book_offers', data: BookOffersRequest): BookOffersResponse;
  async request(command: string, data: Object): Response {
    // implementation
  }

Proposal: Flexible API Response Interface

While moving closer to the Rippled API spec is a goal, there are still times where we want to handle/format/parse the response data into an easier-to-use form. Existing API methods handle some properties automatically, but at the expense of flexibility. If the consumer needs other data that the method does not return, they are out of luck.

Data Objects are simple wrappers around raw API response objects, that give the user access to all raw data from the API response (okay for most properties) plus the same helpful formatting that consumers are already used to.

Here's a partial example of what the Offer class would look like:

class Offer {
  data: OfferRaw;
  constructor(offer: OfferRaw) {
    this.data = offer;
  }
  // return a direction string based on the offer's sell flag
  getDirection(): string {
    return (this.data.flags & flags.Sell) === 0 ? 'buy' : 'sell';
  }
  // return data.taker_gets if the direction is 'buy', otherwise data.taker_pays
  getAccountGets(): Amount;
  // return data.taker_pays if the direction is 'buy', otherwise data.taker_gets
  getAccountPays(): Amount;
  // ...
}

And then here it is in use, flexible enough to handle any properties that the user wants to work with:

import {Offer, parseTimestamp} from '...';

const response = await api.request('account_offers', { account: 'r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59' });

for (const offerRaw of response.offers) {
  // Create an "offer" Data Object for the API response object
  const offer = new Offer(offerRaw);
  // Most data can be read directly from the data object
  const seq = offer.data.seq;
  // Get special formatted data from the API response object
  const direction = offer.getDirection();
  const accountGets = offer.getAccountGets();
  // Some general formatters/parsers can exist for common data types
  const expirationTime = parseTimestamp(offer.data.expiration);
  // Result: Log whatever information you want!
  console.log(`${seq} ${direction}: ${accountGets.value} ${accountGets.currency} (exp. ${expirationTime})`);
}

Feedback?

What do you think? Would love your thoughts @intelliot & @contributors.

intelliot commented 6 years ago

@FredKSchott This looks really good.

I've been thinking about this proposal all morning, and it's very similar to what I had in mind. In fact, it's better - I hadn't thought about using literal values as types. That provides us with type checking for existing APIs while also allowing new commands to be used. Perfect.

We won't need to make breaking changes (existing methods can be phased out at some point during the next year) and further API improvements/simplifications can be made at the rippled layer.

👍 from me.

@wilsonianb @shekenahglory @mDuo13 What do you think?

mDuo13 commented 6 years ago

This is awesome. I'm totally on board with this. One thing that'll require some work beyond the proposed spec is offline signing though. The API should not use the rippled APIs by default when it can do the signing operation without network access. We should continue to support the existing APIs for this.

You've already touched on timestamp conversion. Other formatters that should be supported (this is all implemented already, but may need to be adapted to the new interface):

FredKSchott commented 6 years ago

@intelliot nice! That's good to hear this isn't too far off.

@mDuo13 👍 That's a great point. None of those really map 1:1 with rippled API commands either, so it definitely makes sense to leave the offline logic as-is for now. Where they do make requests, we can refactor those to use this new request() API.

Down the road, it would be nice to make it even more explicit which of those methods are truly offline-only. Right now it's hard to tell which require a connection and what data they'll fetch from it. Also easy to forget which require the "offline" option.

Also great point about Flags, it sounds like they should be handled by the Data Object (ex: every object has a parsed flags property object)

FredKSchott commented 6 years ago

Thanks for the feedback! I'll hopefully find some downtime over the holidays to get started on a few of these

FredKSchott commented 6 years ago

@intelliot @mDuo13 I had a bunch of time over the 🦃 holidays to work on this and have a first branch about ready for PR.

A preview of it is up, but I ran into a problem early on:

Flow doesn't support function overloading. I was so used to using TypeScript for JS typings that I'd just assumed Flow supported this as well. I actually ran into a few different issues while digging into the code base, including some obvious violations & bugs that Flow just wasn't detecting.

Has anyone on the team used TypeScript before? Having used both, TypeScript is really miles ahead of Flow:

Would you consider TypeScript for ripple-lib? This is probably something you all want to talk about as a team, but I think it's the right choice for something complex and financially-focused like ripple-lib, even separate from this larger proposal. I'm happy to discuss offline more as well if you have any questions.

I was able to make the move to TypeScript without too much trouble (two full days of flying without wifi didn't hurt). It involved two main changes to the codebase:

Common.js -> ESM (require() -> import, module.exports -> export) https://github.com/ripple/ripple-lib/compare/develop...FredKSchott:esm TypeScript uses ESM syntax almost exclusively. It's "strict" by default, and a much more standard explicit syntax that is easier for tooling/bundlers/browsers to optimize for. I was able to do this conversion first with the existing Babel setup, separate from any TS work, and I'd recommend this change regardless.

Flow -> TypeScript https://github.com/FredKSchott/ripple-lib/compare/esm...FredKSchott:typescript Flow and TypeScript use an almost exactly similar type format, which is nice. Most of the work consisted of fixing type violations & bugs that had gone undetected in Flow. I've erred on the side of caution and documented any issues that weren't easy fixes with TODOs vs. trying to fix them here and introducing breaking changes.

Let me know what you think, happy to create PRs from these branches if you want to move forward.

FredKSchott commented 6 years ago

@intelliot friendly ping! Would love to hear your thoughts on this. Happy to discuss offline as well.

intelliot commented 6 years ago

Thanks!

Our team hasn't used TypeScript, but I'm looking into it, and I think it's a good language. These branches look like a great start!

I'll reach out to you via email as well.

intelliot commented 6 years ago

This looks good. We would like to move to TypeScript - we'll review PRs when ready.

sublimator commented 6 years ago

TypeScript rocks, you'll never wanna fo back to raw JS :)

FredKSchott commented 6 years ago

Sounds good, I'll get those two branches into PRs

tuloski commented 6 years ago

Dear god no! If you change again all the interfaces now that I have ported all the old ripple-lib code to RippleAPI I'll leave ripple forever and sell all my XRP and spam FUD on reddit and DDoS the validators :).

FredKSchott commented 6 years ago

@tuloski this work is being done specifically so that you can continue to use the current interface. Each method is being kept and rewritten to use the new general request() method internally. If ripple-lib ever did officially deprecate them, they wouldn't need to be removed, just moved to a separate helper library and continue to work as-is.

Also, threats against developers or the projects they work on are not appreciated or helpful. Please learn better Open Source manners if you plan to continue contributing.

tuloski commented 6 years ago

@FredKSchott I put a smile in the comment because I was joking :)

So do you want to keep the old "getAccountInfo", etc, and just re-implement internally with request/requestAll, such that using request one can use the standard rippled request also? Sounds good. I understood the request was going to replace old methods.

Btw the old ripple-lib already had the same interfaces as rippled. Do you know why rippleAPI modified it to have a separate interface? I agree that having the same common format with rippled answers is very good.

FredKSchott commented 6 years ago

@tuloski Sounds good, tone is hard on Github :)

Correct, that's the immediate-term plan. Personally, I'd eventually like to see the current methods pulled out of ripple-lib and moved to a separate library (since they now use the preferred public request() interface) but if this ever happened it would be done specifically to be as easy as possible for existing users.

sublimator commented 6 years ago

haha, I knew adding this extra "RippleAPI" layer would lead to just more trouble and ultimately more to learn :) You can't escape the lower levels when debugging ...

FredKSchott commented 6 years ago

Excited to see progress in #816! Thanks for the reviews @intelliot.

I've been thinking about an added bonus while I've been adding typing info for different api requests/responses:

Right now, the API is documented on the docs site via markdown. We redefine those interfaces in 1. the docs site, 2. rippled, and 3. ripple-lib (either in Flow or TypeScript). Manually keeping all of these in sync adds extra maintenance + risks bugs/developer issues where they get out of date from each other. It would be great to keep a single source of truth that rippled & the docs site & rippled-lib could all reference and test against:

This would be a longer-term project, but the work being done in https://github.com/FredKSchott/ripple-lib/compare/typescript...FredKSchott:request-interface is setting the stage if we end up wanting to go down that road. Those api TypeScript definitions could be converted to JSON Schema and moved to rippled to become the single source of truth.

mDuo13 commented 6 years ago

I've been pushing for a while to get rippled to define JSON Schemas for requests and responses so it could validate input and output. That's not so easy with the way rippled's RPC code is currently architected. But if we did, then yeah, the docs site could easily generate response/request tables from said schemas; the code to do that already exists.

sublimator commented 6 years ago

@mDuo13

It would totally be a worthwhile project, getting the schemas in place for the rippled api, even if they didn't use them to validate right away.

sublimator commented 6 years ago

further API improvements/simplifications can be made at the rippled layer.

The API layer should be separated from rippled. I think things would move a lot faster if you had "dumb" peers (speaking to rippled via binary) implemented in some higher level language, where the integration guys could actually dogfood the API.

As flawed as this "wash over the rippled api quirks" approach was, it was done for a reason. There's a bunch of inconsistencies in the rippled api.

sublimator commented 6 years ago

With a plugin architecture or course ... for community commands ...

sublimator commented 6 years ago

But that would be way too cool and make too many people happy

tuloski commented 5 years ago

If the goal is to remove all the "high level" request and move everything to "low level" request then here we are. But I don't see why the lib cannot provide at the same time high level request as:

Because sincerely making everything a request is not much more than opening a ws and sending the raw request as described in the rippled API. The big problems the user have is in parsing/filtering the data received.

intelliot commented 5 years ago

We want users to be aware of the rippled API and make requests directly. The library does provide helper methods to make it easier to create requests and parse/filter responses.

getBooks is supported with formatBidsAndAsks (coming soon) and getOrderbook (legacy).

I’m not sure what we need to do to support autobridged books, but it’s something I’d like to look into.

Providing helpers for managing subscriptions, maintaining books, and filtering would be great too.

Transaction signing is already supported as well. There’s some code cleanup to do, and it would be great to reduce our dependencies as well.

Thanks for the feedback!

intelliot commented 4 years ago

formatBidsAndAsks is now available. Docs: https://xrpl.org/rippleapi-reference.html#formatbidsandasks

Closing this issue since we've made big strides in this direction, but feel free to open a new issue for any additional 'convenience' helpers that are desired.