interledger-deprecated / ilp-plugin-bells Goto Github PK

interledger/rfcs#80

SonarQube shows 4errors in the ilp-plugin-bells code

all 4 are the same: "Add a "yield" statement to this generator."

Example code missing a yield in src/errors/external-error.js, line 7:
class ExternalError extends BaseError {
handler (ctx, log) {
log.warn('External Error: ' + this.message)
ctx.status = 502
ctx.body = {
id: this.name,
message: this.message,
owner: this.accountIdentifier
}
}
}
src/errors/external-error.js
L7

src/errors/unprocessable-entity-error.js
L6

src/errors/unrelated-notification-error.js
L8

src/lib/factory.js
L117

It seems like if you give the plugin an incorrect or malformatted URL to send a request to (e.g. blah), it will continue retrying the request forever. It may do it with an increasing backoff but it should have an absolute cutoff or some point when it will just give up.

Currently, we can probably expect all five-bells-ledger instances to run version 19.4.2, which means they use version 21.0.2 of five-bells-shared. This means the execution_condition field in FiveBellsTransfer objects should start with cc:

Yet, ilp-plugin-bells version 10.2.6 is not prepending this prefix to the base64 executionCondition that is coming from the plugin interface

I'm still looking for ways to confirm that this is actually what happened, but it seems that this incompatibility was not detected by our integration tests (they only look at the latest version of each component, not on the version of each component that is currently used by ilp-kit), and that this is what brought down the Interledger community network. :/

.send now expects the account field as an ILP address. Transfers emitted from events should translate the five-bells-ledger account URL into an ILP address (here)

Currently, the plugin will fetch an auth token and reconnect with it whenever its connection is dropped. For long-term ledger connections, this is not sufficient. After 7 days, the token will no longer be valid, and so all reconnection attempts will fail. The plugin needs to re-fetch the auth token after some amount of time, so it can retain its subscription.

The plugin keeps trying to ping even after .disconnect() is called.

as specified here

Incoming messages are not validated against the corresponding message schema. It looks like such functionality existed at some point, but is now commented out.

This implementation contradicts this spec.

If the plugin tries to connect to an account that doesn't exist it will throw Failed to resolve ledger URI from account URI. If GETting the account returns a 404 it should throw a more specific error.

From the perspective of the connector calling the getBalance method, the value they care about is the difference between their current balance and the minimum_allowed_balance. Should we return that value for the getBalance call?

This commit in ilp-kit removed the ledgerAccount rel from webfinger which broke login via identifier.

interledger-deprecated/ilp-kit@35b7377#diff-70b4ca4f42e3ae499c1764dd3cf2bea8L121

Logging in by account URI is very unintuitive, so we should add some way to log in knowing only the SPSP identifier.

As I see it, there are two options:

1. Re-add ledgerAccount to the webfinger in ilp-kit.
2. Split the SPSP identifier at the @, webfinger the hostname to get the ledger uri, then insert the account name into the account url template.

Option no 1 is cleaner, but I don't like adding extra rels if they're not strictly needed.

Option no 2 is more complicated, but might be more robust since it is using only commonly used features.

Thoughts?

There are three places where ws.send is called, and none of them use an error callback.

In particular, if the ledger was restarted but the process in which the plugin runs was not, I would expect all subsequent ws.send attempts to fail. When this happens, it should close the current WebSocket and try to connect a new one.

I'll assign this to myself and have a look at it today/tomorrow.

Including five-bells-shared causes ed25519 to be compiled, which is totally unnecessary considering that ilp-plugin-bells is only using a couple of error types.

src/lib/plugin.js has two duplicate sections. The first is from line 450-475, which repeats from line 497 to 522.

Right now the plugin does string concatenation. We should support setups where the endpoints are hosted in different locations by using the values returned by the metadata endpoint.

Depends to some extent on interledger-deprecated/five-bells-ledger#326

We shouldn't need to configure plugin bells with the prefix because it should be able to get it from the ledger

The readme only gives an example of how to use this plugin as an admin, to listen to all ledger messages. Even if this package just implements the standard ILP plugin API, we should still document what the options for constructor arguments are.

Right now it retries 404 errors but there's no point if the account doesn't exist

Reducing the dependency size would make it more reasonable to include ilp-plugin-bells by default in libraries like ilp.

According to cost-of-modules, we could dramatically reduce the size by using the broken out lodash packages that we need and switching from co-request to superagent (which is 1.13 Mb instead of 4.42):

┌────────────────┬─────────────┬───────┐
│ name           │ children    │ size  │
├────────────────┼─────────────┼───────┤
│ lodash         │ 0           │ 4.84M │
├────────────────┼─────────────┼───────┤
│ co-request     │ 63          │ 4.42M │
├────────────────┼─────────────┼───────┤
│ ws             │ 2           │ 0.21M │
├────────────────┼─────────────┼───────┤
│ reconnect-core │ 2           │ 0.17M │
├────────────────┼─────────────┼───────┤
│ debug          │ 1           │ 0.09M │
├────────────────┼─────────────┼───────┤
│ path-to-regexp │ 1           │ 0.06M │
├────────────────┼─────────────┼───────┤
│ eventemitter2  │ 0           │ 0.05M │
├────────────────┼─────────────┼───────┤
│ co             │ 0           │ 0.03M │
├────────────────┼─────────────┼───────┤
│ 8 modules      │ 66 children │ 9.79M │
└────────────────┴─────────────┴───────┘

The connect method is currently used as an asynchronous constructor of sorts. There are a lot of paths through this code and many are probably not safe.

• Move asynchronous options resolution, sanitization and normalization into a separate method (currently in connect.)
• Before connecting successfully, much of the plugin's internal state is incomplete, so it should refuse all calls to its other methods until connect has been called successfully.

I think it would make sense for errors specified in the plugin interface specification to be extracted from this project into a plugin error module since they will be used in every plugin implementation.

I was confused by this: https://github.com/interledgerjs/ilp-plugin-bells/blame/master/src/lib/plugin.js#L30

I would expect the ledger to be called something ...Ledger and the plugin for it to be called something ...Plugin, more in line with for instance https://github.com/interledgerjs/ilp-plugin-virtual/blob/master/src/lib/plugin.js#L27.

Right now the plugin does not do anything with rejected transfers unless they had a cancellation_condition_fulfillment

Maybe this is already in progress, but didn't see any issue/PR related:

The plugin is currently outdated compared to RFC-4

main actions:

• remove the connectors logic
• handle the events

cc @justmoon @adrianhopebailie @emschwartz

Should match ledgerInfo

If the plugin is created with only account and password, the requestCredentials function will send no authentication during the connect function. Passing the username into the plugin parameters appears to fix this.

Hi guys,

Are you not missing 'use strict' in some of the files of this plugin?

model/transferlog.js
util/log.js
model/balance.js
test/helpers/sqliteStore.js
test/helpers/objStore.js

At least according to the spec: https://github.com/interledger/rfcs/blob/master/0004-ledger-plugin-interface/0004-ledger-plugin-interface.md#getaccount

It should probably throw/reject if the plugin isn't connected

The plugin is currently implemented here:

https://github.com/interledger/five-bells-connector/blob/1a0563d94351cee2f49292033ec6be898ea5e90d/src/ledgers/five-bells-ledger.js

Save this as test.js:

// const log = require('why-is-node-running') // should be your first require
const FiveBellsLedgerPlugin = require('ilp-plugin-bells')

let plugin = new FiveBellsLedgerPlugin({
  account: 'https://red.ilpdemo.org/ledger/accounts/alice',
  password: 'alice'
});

plugin.connect().then(() => {
  console.log('connected!')
  return plugin.disconnect()
}).then(() => {
  console.log('disconnected, expecting process to exit now...')
  plugin = null
  // log() // logs out active handles that are keeping node running
})

Now run (the --expose-internals is needed because of mafintosh/why-is-node-running#7):

npm install ilp-plugin-bells why-is-node-running
node --expose-internals test.js

You will see the process hangs after the 'disconnected, expecting process to exit now...' statement.
Unfortunately, if you uncomment the first line, the process crashes with an error in net.js, so I haven't been able yet to acually use why-is-node-running to find out why node is running..

@sentientwaffle any idea why the node process keeps running even after you call plugin.disconnect()? Seems to me it's probably related to the whole WebSocket reconnect logic, which is now properly complex in https://github.com/interledgerjs/ilp-plugin-bells/blob/master/src/lib/plugin.js#L211-L330

currently just returns null

Connector is calling getPrefix which throws an exception.

Here's the plugin-bells instance when the getPrefix is called.

FiveBellsLedger {
  _events:
   { '_rpc:notification': [Function],
     incoming_transfer: [Function],
     incoming_prepare: [Function],
     incoming_fulfill: [Function],
     incoming_cancel: [Function],
     incoming_reject: [Function],
     outgoing_transfer: [Function],
     outgoing_prepare: [Function],
     outgoing_fulfill: [Function],
     outgoing_cancel: [Function],
     outgoing_reject: [Function],
     incoming_message: [Function] },
  newListener: false,
  configPrefix: 'us.usd.red.',
  host: null,
  account: 'https://red.ilpdemo.org/ledger/accounts/connie',
  username: undefined,
  identifier: undefined,
  credentials:
   { account: 'https://red.ilpdemo.org/ledger/accounts/connie',
     username: undefined,
     password: '***',
     cert: undefined,
     key: undefined,
     ca: undefined },
  connector: null,
  debugReplyNotifications: false,
  rpcId: 1,
  info: null,
  connection: null,
  connected: false,
  ws: null,
  urls: null }

Here's the exception

2016-12-01T01:23:35.760Z connector:app error Error: Prefix has not been set
    at FiveBellsLedger.getPrefix (/var/app/five-bells-wallet/node_modules/ilp-plugin-bells/src/lib/plugin.js:328:29)
    at RouteBroadcaster._crawlClient (/var/app/five-bells-wallet/node_modules/ilp-connector/src/lib/route-broadcaster.js:100:45)
    at next (native)
    at onFulfilled (/var/app/five-bells-wallet/node_modules/co/index.js:65:19)
    at /var/app/five-bells-wallet/node_modules/co/index.js:54:5
    at co (/var/app/five-bells-wallet/node_modules/co/index.js:50:10)
    at toPromise (/var/app/five-bells-wallet/node_modules/co/index.js:118:63)
    at Array.map (native)
    at arrayToPromise (/var/app/five-bells-wallet/node_modules/co/index.js:154:26)
    at toPromise (/var/app/five-bells-wallet/node_modules/co/index.js:120:49)
    at next (/var/app/five-bells-wallet/node_modules/co/index.js:99:29)
    at onFulfilled (/var/app/five-bells-wallet/node_modules/co/index.js:69:7)
    at /var/app/five-bells-wallet/node_modules/co/index.js:54:5
    at co (/var/app/five-bells-wallet/node_modules/co/index.js:50:10)
    at toPromise (/var/app/five-bells-wallet/node_modules/co/index.js:118:63)
    at next (/var/app/five-bells-wallet/node_modules/co/index.js:99:29)
npm run start-prod-connector exited with code 0

Right now .connect resolves when the reconnect-core emits a connect event but it should actually wait until the ws emits an open event. It should reject with an UnreachableError, as per the spec, if subscribing fails.

These tests should be failing because the websocket endpoint it tries to connect to doesn't even exist (it's not stubbed right now)

When receiving a message or transfer from and to the same user, the factory should emit only one event, however, it will currently emit two.

Suggested fix:

Add accounts = uniq(accounts) in factory.js, line 98 where uniq is lodash/uniq.

This issue currently breaks sending payments to the connector user in ILP Kit.

This repo is included in the integration tests so they should be run on Circle

Unsure which one to use