interledgerjs / ilp-connector

Reference implementation of an Interledger connector.

Other

135 stars 53 forks source link

feat: broaden scope of backend service #479

Closed tangjeff0 closed 5 years ago

tangjeff0 commented 5 years ago

controllers/ilp-prepare - change submitPayment to submitPacket: always submit to backend whether packet type is fulfill, prepare, reject
types/backend - add more params to SubmitPaymentParams: backend should receive more data
services/backend - replace this.getInfo with this.accounts: backend should have access to more of the accounts api
backends ecb, one-to-one, randomizer: update from using this.getInfo to this.accounts

codecov-io commented 5 years ago

Codecov Report

Merging #479 into master will decrease coverage by 0.09%. The diff coverage is 55.55%.

@@            Coverage Diff            @@
##           master     #479     +/-   ##
=========================================
- Coverage   73.62%   73.53%   -0.1%     
=========================================
  Files          44       44             
  Lines        2059     2067      +8     
  Branches      332      334      +2     
=========================================
+ Hits         1516     1520      +4     
- Misses        543      547      +4

Impacted Files	Coverage Δ
src/controllers/ilp-prepare.ts	`82.92% <25%> (-6.27%)`	:arrow_down:
src/services/rate-backend.ts	`96.66% <80%> (-3.34%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a7c4bcd...d61e0aa. Read the comment docs.

sharafian commented 5 years ago

There are a lot of breaking changes here that could be done non-breakingly. These are good features though, especially because they're informed by real-world usage of the connector.

Instead of replacing getInfo with accounts you could add accounts and still keep getInfo
submitPacket could run on prepare/fulfill/reject, and on fulfill if submitPacket is not specified it can fall back to submitPayment

tangjeff0 commented 5 years ago

This might be a dumb question, but where are the breaking changes? I thought I modified the backends (ecb, one-to-one, randomizer) such that they use submitPacket and this.accounts.getInfo rather than submitPayment and this.getInfo, respectively.

kincaidoneil commented 5 years ago

For example, we wrote our own custom backend. Since it imports and implements the backend interface from the connector, this change would break it since it relies on getInfo, and would introduce type errors.

tangjeff0 commented 5 years ago

With help from @sharafian and @kincaidoneil :

in types/backend: accounts property optional, getInfo method unchanged
in types/backend: submitPacket method optional, submitPayment method unchanged
in controllers/ilp-prepare: call submitPacket on prepare/fulfill/reject if method exists, otherwise call submitPayment on fulfill
in services/rate-backend: on construction, assign this.submitPayment to be this.backend.submitPayment, otherwise, leave it undefined

Hopefully these changes improve the extensibility of custom backends, while maintaining backwards-compatibility.

tangjeff0 commented 5 years ago

Awesome - can @sentientwaffle also take a look at these two PRs? They follow the paradigm you initially suggested of passing in a server object to the respective constructors.