New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to ILPv4 + LPI2 #401

Merged
merged 48 commits into from Jan 19, 2018

Conversation

Projects
None yet
6 participants
@justmoon
Copy link
Contributor

justmoon commented Dec 18, 2017

@emschwartz
Copy link
Member

emschwartz left a comment

Overall LGTM, but I think @sentientwaffle should also review this because he's more familiar with a lot of the code this PR touches

"five-bells-shared": "^25.1.0",
"ilp": "~11.2.0",
"ilp-packet": "~1.3.0",
"ilp-packet": "^2.0.0",
"ilp-plugin-bells": "^15.0.0",

This comment has been minimized.

@emschwartz

emschwartz Dec 18, 2017

Member

Should we stop including plugin bells?

const fulfillPromise = promiseRetry(retryOpts, function (retry, number) {
return Promise.resolve(plugin.fulfillCondition(sourceTransferID, fulfillment, fulfillmentData))
.catch(function (err) {
if (shouldRetry(err)) {

This comment has been minimized.

@emschwartz

emschwartz Dec 18, 2017

Member

Should we add fulfillment retry logic to the ilp-compat-plugin?

This comment has been minimized.

@justmoon

justmoon Dec 18, 2017

Contributor

I do think that fulfillment retries should be in the plugin because it depends on the ledger protocol if and how you want to retry.

Should it be in ilp-plugin-compat? I don't think it's worth the effort. Retry logic is an optimization, it's only to minimize unnecessary connector losses. I'd say we migrate to LPI2 and then add retry logic per plugin. Different plugins may want to handle retries quite differently, depending on whether they're TCP or UDP based, session or request based or to deal with some other ledger/transport-specific quirks.

@@ -22,40 +20,6 @@ function getPairs (arr) {
}, [])
}

/**
* Deterministically generate a UUID from a secret and a public input.

This comment has been minimized.

@emschwartz

emschwartz Dec 18, 2017

Member

Right now Mojaloop depends on having the same transfer ID across all of the ledgers. Taking the transfer id out of the LPI will make it more difficult to upgrade the connector used in that project. Do we have any alternative way of attaching the transfer ID from the previous ledger and making the connector use the same one on the outgoing transfer, or do we need to just make Mojaloop not depend on having the same transfer IDs?

@sentientwaffle
Copy link
Contributor

sentientwaffle left a comment

LGTM except for a couple minor notes

@@ -149,6 +149,8 @@ function getLocalConfig () {
features.debugReplyNotifications =
Config.castBool(Config.getEnv(envPrefix, 'DEBUG_REPLY_NOTIFICATIONS'))

const account = Config.getEnv(envPrefix, 'ILP_ADDRESS') || 'unknown'

This comment has been minimized.

@sentientwaffle

sentientwaffle Dec 18, 2017

Contributor

Add ILP_ADDRESS to the README.

@@ -39,9 +39,10 @@ class RouteBroadcaster {

this.autoloadPeers = config.autoloadPeers
this.defaultPeers = config.peers
// peersByLedger is stored in the form { ledgerPrefix ⇒ { connectorAddress ⇒ true } }
// peersByLedger is stored in the form { ledgerPrefix ⇒ true }

This comment has been minimized.

@sentientwaffle

sentientwaffle Dec 18, 2017

Contributor

s/peersByLedger/peers

// timeout the plugin.sendRequest Promise just so we don't have it hanging around forever
timeout: this.routeBroadcastInterval
})
const broadcastPromise = this.ledgers.getPlugin(adjacentLedger).sendRequest({

This comment has been minimized.

@michielbdejong

michielbdejong Dec 18, 2017

Contributor

Does this.ledgers.getPlugin(adjacentLedger) return an LPI1 or an LPI2 interface? Because the sendRequest method, needed for broadcasting routes, seems to be absent in LPI2?

This comment has been minimized.

@emschwartz

emschwartz Dec 18, 2017

Member

It's not in the spec but it is in the ilp-compat-plugin as a deprecated feature

This comment has been minimized.

@sentientwaffle

This comment has been minimized.

@michielbdejong

michielbdejong Dec 18, 2017

Contributor

So then this PR will break route broadcasts?

This comment has been minimized.

@sentientwaffle

sentientwaffle Dec 18, 2017

Contributor

LPI1 plugins will work thanks to compat; LPI2 plugins will be broken.

This comment has been minimized.

@emschwartz

emschwartz Dec 19, 2017

Member

They should be switched to use either interledger payments or the internet

This comment has been minimized.

@sharafian

sharafian Dec 19, 2017

Contributor

in a similar way to @justmoon 's DHCP on ILP proposal (for asking your peer to assign you an ILP address), it would be easy to send a payment to your peer for which the destination is something like peer.broadcast, and the condition is either trivially fulfillable (if you're paying to send broadcasts) or unfulfillable (if you're not paying to send broadcasts). I don't see why that's worse/harder than using messaging.

This comment has been minimized.

@michielbdejong

michielbdejong Dec 20, 2017

Contributor

Sure, that's what plugin-xrp-escrow already does. And if a BTP connection is available between the connectors then they can just use a BTP Message packet. But that's what plugin.sendRequest already nicely abstracts, and we agree that ilp-connector needs it, so then instead of removing code from the stack, we have just split the ledger plugin into a separate transfer plugin and messaging plugin, which is just moving code around.

This comment has been minimized.

@sharafian

sharafian Dec 20, 2017

Contributor

We don't actually need a messaging plugin anymore though, because transfers are expected to be just as fast as messages. And setting the amount to 1 means that the impact on your liquidity is negligible. This just lets us keep plugins as simple as possible, which should make them less buggy and more secure

This comment has been minimized.

@michielbdejong

michielbdejong Dec 20, 2017

Contributor

Ben and I discussed this on Slack and reduced this question to interledger/rfcs#357

@michielbdejong michielbdejong referenced this pull request Dec 18, 2017

Merged

Update to LPI2 #122

@justmoon justmoon force-pushed the refactor/st-lpi2 branch from 4cc5e35 to 6279cac Dec 19, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 19, 2017

Codecov Report

Merging #401 into master will decrease coverage by 10.27%.
The diff coverage is 74.54%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #401       +/-   ##
===========================================
- Coverage   84.82%   74.54%   -10.28%     
===========================================
  Files          22       51       +29     
  Lines         336     1756     +1420     
  Branches       49      278      +229     
===========================================
+ Hits          285     1309     +1024     
- Misses         51      447      +396
Impacted Files Coverage Δ
src/errors/invalid-packet-error.ts 100% <100%> (ø)
src/services/routing-table.ts 100% <100%> (ø)
src/errors/invalid-amount-specified-error.ts 100% <100%> (ø)
src/errors/unacceptable-amount-error.ts 100% <100%> (ø)
src/errors/unreachable-error.ts 100% <100%> (ø)
src/errors/unacceptable-expiry-error.ts 100% <100%> (ø)
src/lib/ilp-errors.ts 100% <100%> (ø)
src/errors/no-route-found-error.ts 100% <100%> (ø)
src/common/log.ts 100% <100%> (ø)
src/errors/invalid-fulfillment-error.ts 100% <100%> (ø)
... and 92 more

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 25963d7...bb8f737. Read the comment docs.

@justmoon justmoon force-pushed the refactor/st-lpi2 branch from 9d17bb5 to b23fe4a Dec 26, 2017

@emschwartz
Copy link
Member

emschwartz left a comment

Looks awesome to me. @sentientwaffle should definitely review this again

writer.writeVarOctetString(Buffer.from(sourceAccount, 'ascii'))
writer.writeUInt8(info.currencyScale)
writer.writeVarOctetString(Buffer.from(info.currency, 'utf8'))
return writer.getBuffer()

This comment has been minimized.

@emschwartz

emschwartz Dec 27, 2017

Member

This encoding should probably be added to the ilp-packet module

This comment has been minimized.

@justmoon

justmoon Dec 27, 2017

Contributor

Yeah, I wasn't sure where to put it. It's not ilp-packet layer, so I don't think ilp-packet is actually the right place unless we turn that into ilp-formats. Plus - is it really worth modularizing four lines of code?

This comment has been minimized.

@emschwartz

emschwartz Dec 27, 2017

Member

Fair enough

const { nextHop, nextHopPacket } = await this.routeBuilder.getNextHopPacket(sourceAccount, parsedPacket)

log.debug('sending outbound ilp prepare. destination=%s amount=%s', destination, nextHopPacket.amount)
const result = await this.sendDataToPeer(nextHop, IlpPacket.serializeIlpPrepare(nextHopPacket))

This comment has been minimized.

@emschwartz

emschwartz Dec 27, 2017

Member

Shouldn't we modify the ILP packet in place rather than re-encoding? (That could also be a separate PR though)

This comment has been minimized.

@justmoon

justmoon Dec 27, 2017

Contributor

Imho, that's premature optimization. Not long ago, we were ok going with a text-based format that would have been an order of magnitude slower. It's just not worth worrying about trying to get zero-copy performance at this stage. There are much lower hanging fruit.


// TODO: Is this the right error code? Maybe we should pass on the error we
// we received where possible?
this.ilpErrorCode = codes.T00_INTERNAL_ERROR

This comment has been minimized.

@emschwartz

emschwartz Dec 27, 2017

Member

I'm not sure these errors will be temporary if other connectors just stop supporting ILQP

This comment has been minimized.

@justmoon

justmoon Dec 27, 2017

Contributor

Good point - what's the right error code here? Or should we add one?

(I ran into a few cases where I was like: might be good to add a code for this - or make an existing code less specific.)

logging: log.debug,
omitNull: true,
// All transactions should be done with isolation level SERIALIZABLE
// TOOD:

This comment has been minimized.

@emschwartz

emschwartz Dec 27, 2017

Member

What's this TODO for?

This comment has been minimized.

@justmoon

justmoon Dec 27, 2017

Contributor

This "TOOD" was going to say: So we still need SERIALIZABLE isolation level. (My guess is no.)

}

handleMoney (amount) {
// TODO: Implement balance logic

This comment has been minimized.

@emschwartz

emschwartz Dec 27, 2017

Member

This should be implemented ASAP after this PR is merged

This comment has been minimized.

@justmoon

justmoon Dec 27, 2017

Contributor

Yeah. I think that's a job for @sharafian - he's written that code about 15 times already since it used to be in each plugin.


// Make sure that neither amount exceeds 15 significant digits.
if (rate.gt(1)) {
return new LiquidityCurve([ [0, 0], [ PROBE_AMOUNT / rate, PROBE_AMOUNT ] ])

This comment has been minimized.

@emschwartz

emschwartz Dec 27, 2017

Member

Shouldn't this be using bignumber.js' math functions?

This comment has been minimized.

@justmoon

justmoon Dec 27, 2017

Contributor

Copied the existing code to make sure the results are exactly the same - I still like that idea so we don't have to update the tests as much. But we'll see once we get to updating the integration tests how it'll play out.

README.md Outdated
}, {
"targetPrefix": "cny.",
"connectorLedger": "ilpdemo.red."
"connectorAccount": "lpdemo.red.cny_connector"
"peerAddress": "ilpdemo.red."

This comment has been minimized.

@sentientwaffle

sentientwaffle Dec 28, 2017

Contributor

I think the peerAddress for this route should be ilpdemo.blue. (or something other than ilpdemo.red.), otherwise it will be caught by the targetPrefix: "" route anyway.


async start () {
try {
await this.reloadLocalRoutes()

This comment has been minimized.

@sentientwaffle

sentientwaffle Dec 28, 2017

Contributor

this.reloadLocalRoutes is sync, no need to await.

await new Promise(resolve => setTimeout(resolve, this.config.routeBroadcastInterval))

try {
await this.reloadLocalRoutes()

This comment has been minimized.

@sentientwaffle

sentientwaffle Dec 28, 2017

Contributor

this.reloadLocalRoutes is sync, no need to await.

* @param {String} params.destination_ledger The URI of the destination ledger
* @param {String} params.source_currency The source currency
* @param {String} params.destination_currency The destination currency
* @param {String} sourceAccount The URI of the source ledger

This comment has been minimized.

@sentientwaffle

sentientwaffle Dec 28, 2017

Contributor

These are ILP addresses, not ledger URIs (the code is correct; the comment is wrong).

* @param {String} params.destination_ledger The URI of the destination ledger
* @param {String} params.source_amount The amount of the source asset we want to send
* @param {String} params.destination_amount The amount of the destination asset we want to send
* @param {String} params.sourceAccount The URI of the source ledger

This comment has been minimized.

@sentientwaffle

sentientwaffle Dec 28, 2017

Contributor

sourceAccount and destinationAccount are ILP addresses.

This comment has been minimized.

@justmoon

justmoon Jan 3, 2018

Contributor

Not anymore after config refactor.

})
}

this.backend.submitPayment({

This comment has been minimized.

@sentientwaffle

sentientwaffle Dec 28, 2017

Contributor

Should the submitPayment call be inside of the result[0] === TYPE_ILP_FULFILL block? Otherwise it might be submitted even if sendDataToPeer replied with a rejection.


if (nextSegmentEnd === -1) {
prefix = address
return false

This comment has been minimized.

@sentientwaffle

sentientwaffle Dec 28, 2017

Contributor

No need to return false, just return.

const PrefixMap = require('./prefix-map')
const log = require('../common').log.create('routing-peer')

const PEER_PROTOCOL_PREFIX = 'peer'

This comment has been minimized.

@sentientwaffle

sentientwaffle Dec 28, 2017

Contributor

ilp-prepare.js declares this as 'peer.'.

const newRoutes = routes.filter(route => route.epoch > this.epoch && route.nextHop !== this.address).map(route => ({
source_ledger: this.address,
destination_ledger: route.prefix,
min_message_window: 1,

This comment has been minimized.

@sentientwaffle

sentientwaffle Dec 28, 2017

Contributor

This should reference the configured minMessageWindow.

This comment has been minimized.

@justmoon

justmoon Jan 3, 2018

Contributor

Removed for now.

data: {
new_routes: newRoutes,
hold_down_time: holdDownTime,
unreachable_through_me: unreachableAccounts,

This comment has been minimized.

@sentientwaffle

sentientwaffle Dec 28, 2017

Contributor

unreachableThroughMe, not unreachableAccounts.


const store = this.store.getPluginStore(accountAddress)

creds.options.prefix = accountAddress

This comment has been minimized.

@sentientwaffle

sentientwaffle Jan 2, 2018

Contributor

Is this line necessary (see line 93 prefix: accountAddress + '.',)? If it is, shouldn't the prefix be set with a + '.' suffix?

This comment has been minimized.

@justmoon

justmoon Jan 3, 2018

Contributor

Removed

features.debugReplyNotifications =
Config.castBool(Config.getEnv(envPrefix, 'DEBUG_REPLY_NOTIFICATIONS'))

this.address = Config.getEnv(envPrefix, 'ILP_ADDRESS') || ''

This comment has been minimized.

@sentientwaffle

sentientwaffle Jan 2, 2018

Contributor

This parameter should be documented in the README.

This comment has been minimized.

@justmoon

justmoon Jan 3, 2018

Contributor

Yeah, README needs a major rewrite...

@justmoon justmoon force-pushed the refactor/st-lpi2 branch from 4bc732c to 5ced0f2 Jan 3, 2018

@justmoon justmoon force-pushed the refactor/st-lpi2 branch from 5ced0f2 to 913ca83 Jan 3, 2018

@justmoon justmoon force-pushed the refactor/st-lpi2 branch 4 times, most recently from f7cf20e to 3a3cc48 Jan 16, 2018

@justmoon justmoon force-pushed the refactor/st-lpi2 branch from 3a3cc48 to 5d0867d Jan 16, 2018

README.md Outdated
| -------------- | ------- | ------------------------------------------------------------------------ |
| `[]` | object | Object describing middleware instance. |
| `[].type` | string | NPM module that should be `require`d to load the middleware constructor. |
| `[].priority` | integer | Priority at which this middleware should be inserted. Must be unique. |

This comment has been minimized.

@sentientwaffle

sentientwaffle Jan 16, 2018

Contributor

Is this field implemented? I don't see it used in the code.

This comment has been minimized.

@justmoon

justmoon Jan 16, 2018

Contributor

Nope, it was from a previous version, I'll take it out!

This comment has been minimized.

@justmoon

justmoon Jan 16, 2018

Contributor

Removed.

README.md Outdated
| `*.assetCode` | string | Currency code or other asset identifier that will be passed to the backend to select the correct rate for this account. |
| `*.assetScale` | integer | Interledger amounts are integers, but most currencies are typically represented as fractional units, e.g. cents. This property defines how many Interledger units make up one regular units. For dollars, this would usually be set to 9, so that Interledger amounts are expressed in nanodollars. |
| `*.balance` | object | _Optional_ Defines whether the connector should maintain and enforce a balance for this account. This setting is enforced by the built-in `balance` middleware. |
| `*.balance.maximumDebt` | string | How much money (in atomic units) this counterparty is allowed to owe us before we start rejecting their packets. Each incoming ILP prepare packet increases the amount owed, each incoming settlement lowers it. Provided as an integer in a string. |

This comment has been minimized.

@sentientwaffle

sentientwaffle Jan 18, 2018

Contributor

should be balance.maximum (or the balance middleware needs to change).

@justmoon justmoon force-pushed the refactor/st-lpi2 branch from 9030e87 to 28c6873 Jan 19, 2018

@justmoon justmoon force-pushed the refactor/st-lpi2 branch from 28c6873 to b33434a Jan 19, 2018

@justmoon justmoon force-pushed the refactor/st-lpi2 branch from 8a8a992 to bb8f737 Jan 19, 2018

@justmoon justmoon merged commit 6c72764 into master Jan 19, 2018

1 of 2 checks passed

ci/circleci CircleCI is running your tests
Details
licence/cla Contributor License Agreement is signed.
Details
@emschwartz

This comment has been minimized.

Copy link
Member

emschwartz commented Jan 19, 2018

Woohoo! What a monster PR. Nicely work @justmoon

@justmoon justmoon referenced this pull request Feb 21, 2018

Open

Phase 0: Minimum Viable Network (Pong) #7

5 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment