Skip to content
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

Ledger Plugin Interface v2 #347

Merged
merged 11 commits into from
Jan 20, 2018
Merged

Ledger Plugin Interface v2 #347

merged 11 commits into from
Jan 20, 2018

Conversation

justmoon
Copy link
Member

@justmoon justmoon commented Dec 2, 2017

Please note the changes below are in roughly chronological order, later changes may revise earlier ones.

Ledgers are a type of connector (Interledger enlightenment)

@sharafian and I realized a while ago that ILP could be simplified by treating ledgers as another type of connector. This later came to be known as "Interledger enlightenment".

Plugins that represent ledgers have to do a simple form of routing. If the ILP destination address starts with the ledger prefix, they should forward the transfer to the account mentioned in the destination address (usually the first segment after the ledger prefix). If it doesn't, they should forward the payment to the default connector. This new behavior no longer allows a connector to reach multiple other connectors through one plugin. If that is desired, the connector should create a separate plugin per peer connector.

  • Remove connectors: All payments to a plugin go through the default connector if the ILP destination is not on the ledger
  • Remove account/from/to: It's the plugin's responsibility to determine the destination
  • Remove prefix: Ledger prefixes are now handled internally by the plugin

Ephemeral transfers

Fulfillments and rejections are simply returned by sendTransfer calls. Now that we envision Interledger transfers to always be relatively small, we can slightly relax some of the guarantees around the individual transfer. Based on work by @michielbdejong and @emschwartz.

  • Change sendTransfer to return Promise<FulfillmentInfo> or reject with InterledgerRejectionError
  • Add registerTransferHandler and deregisterTransferHandler (if no handler is registered, plugin should reject all incoming transfers)
  • Remove noteToSelf: We can now use local state in the sendTransfer caller function
  • Remove fulfillCondition: Instead, return FulfillmentInfo
  • Remove rejectIncomingTransfer: Instead, throw an InterledgerRejectionError
  • Remove getFulfillment: With short-lived transfers, the need to retrieve a fulfillment for the purpose of retrying later is no longer relevant

LPI is not a different layer than ILP

The ILP packet has become so simple that it no longer makes sense to encapsulate it with the transfer object. Based on #270 by @adrianhopebailie and further work by @emschwartz.

  • Remove ilp from Transfer, FulfillmentInfo
  • Add destination:String, data:Buffer to Transfer
  • Add data:Buffer to FulfillmentInfo

No optimistic mode

As suggested by @emschwartz, optimistic mode can be simulated using Optimistic-over-Universal (OoU).

  • Remove *_transfer events
  • Make executionCondition and expiresAt mandatory for transfers

Middleware provides better extensibility

As discovered by @emschwartz, many use cases of plugins can be better represented using middleware.

  • Remove *_{prepare,fulfill,reject,cancel} events: These can be provided by a middleware that emits such events, if desired
  • Remove *_{request,response} events

Quote by probing

Instead of a dedicated quoting protocol, we quote by sending a probe payment. See #309.

  • Remove sendRequest: We no longer need a dedicated request/response feature

Bandwidth instead of balances

@sharafian made the point that ILP participants only care about their balances insofar as they affect their ability to send payments. In fact, balances are just a special case of a function that takes a payment and returns whether the payment is allowed or not. We can model this more generically by treating it similarly to IP bandwidth. IP doesn't track how much bandwidth is available but instead returns an error when packets are coming through too quickly.

  • Remove getBalance
  • Remove minBalance/maxBalance

Cleanup LPI errors

ILP/LPI errors are a bit of a mess right now.

  • Define an InterledgerRejectionError to be a proper JavaScript error object
  • Change forwardedBy to be an array to match the ILP packet
  • Make message a local property since it does not exist in the ILP error packet and can therefore not be forwarded

Fixes #332.

ILDCP replaces LedgerInfo

It's not a plugin concern to configure the client. This can be done via messages sent on the Interledger layer.

  • Remove getInfo, getAccount and LedgerInfo
  • Remove info_change event

Conditional transfers are an Interledger concern

See #359. We don't need to enforce conditions on the ledger layer. This can be done on the Interledger layer.

  • Remove all mentions of executionCondition, expiresAt
  • Replace transfer: Transfer with amount: string
  • Replace sendTransfer(transfer: Transfer): Promise<FulfillmentInfo> with sendMoney(amount: string): Promise<void>
  • Replace sendRequest(message: Message): Promise<Message> with sendData(data: Buffer): Promise<Buffer>
  • Replace [de]registerRequestHandler and [de]registerTransferHandler with [de]registerDataHandler and [de]registerMoneyHandler

Interledger rejections should be emitted on the Interledger layer only

The plugin previously emitted some Interledger rejections. The plugin shouldn't even know what Interledger is. All errors emitted by the plugin are now plain JavaScript exceptions.

  • Remove InterledgerRejectionError

Other changes

@justmoon
Copy link
Member Author

justmoon commented Dec 2, 2017

Created a module that can turn an LPI1 plugin into an LPI2 plugin, see interledgerjs/ilp-compat-plugin.

@justmoon
Copy link
Member Author

justmoon commented Dec 2, 2017

LPI is not a different layer than ILP

Having some second thoughts about this one. It seems like we're losing extensibility in a pretty major way. In ILPv1, we can add things to the ILP packet which some connectors understand in such a way that it is ignored by the receiver and other connectors that don't support it.

For example, in ILPv1 we could have added a hop limit in a non-breaking way. With this, we can't because there is no way to attach ILP layer information beyond the two natively supported fields (destination and data).

That's fine if we think we never need to extend ILP. But that's a hell of a thing to say.

Edit: OTOH, less extensibility also means less potential for shenanigans.

@michielbdejong
Copy link
Contributor

Why not use http calls instead of JavaScript function calls?
I think previously the answer was "because fulfillments travel in the opposite direction and so we need to listen for events".

@justmoon
Copy link
Member Author

justmoon commented Dec 3, 2017

Why not use http calls instead of JavaScript function calls?

What do you mean by HTTP calls? You mean the plugin should expose a local HTTP server and the connector would call it? That would add a lot of overhead for no (?) benefit?

Copy link
Collaborator

@adrianhopebailie adrianhopebailie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some editorial comments but nothing blocking

For a detailed description of these properties, please see [`Class: Transfer`](#class-transfer).

#### LedgerPlugin#registerTransferHandler
<code>ledgerPlugin.registerTransferHandler( **transferHandler**: ( transfer: [Transfer](#class-transfer) ) ⇒ Promise&lt;[Message](#class-message)> ) ⇒ null</code>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Message -> FulfillmentInfo

'2017-12-02T11:51:26.627Z'
```

#### Transfer#custom
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me how this differs from data if the fields MUST be passed to the receiver.

My assumption is that the plugin will know what needs to go to the receiver and what doesn't based on the underlying connection type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like custom is either for the ledger plugin itself, or for the next connector, whereas the data is end-to-end data that should be treated as opaque by the intermediaries and must be passed on.

Copy link
Member Author

@justmoon justmoon Dec 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me how this differs from data

@emschwartz is correct.

data is passed on by connectors and plugins. custom is only passed on by plugins (generally speaking), but not by connectors. Although there are exceptions. (For example, I'm using custom.ilpLegacyAmount to pass on the ILP amount in my connector refactor.)

I'll see if I can improve the wording.

Edit: Primarily, this seems to be an issue with overloading the terms "sender" and "receiver". When I say receiver of the transfer, I mean the local receiver, i.e. the next hop. When I say receiver of the payment, I generally mean the ILP receiver, i.e. the last hop. But I can see how that's superconfusing. Perhaps we should have different terms for the local receiver. Maybe just "local receiver"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry about having something that is intended for the next connector (local receiver) mixed in with something for the plugin.

This implies the plugin must know how to differentiate between the two (and only send the data intended for the next connector) and that the caller must have some knowledge of the underlying plugin (i.e. does it support X in custom).

This also suggests that we've simply moved the implementation challenges of sendMessage into sendTransfer. If I have a ledger that doesn't support messaging I need to establish a communications channel specifically for sending this data in parallel to the transfers.

Could we split custom into pluginData and peerData?

As a caller, if I have no idea what plugin I am talking to, I can still safely put anything serializable into peerData and know it will get to my peer.

I will only use pluginData if I know what plugin I am speaking to or if some of the data in there becomes standard and I expect all plugins to understand it.


If the `custom` data is too large, the ledger plugin MUST reject with a `MaximumCustomDataSizeExceededError`.

Note that connectors MAY forward some fields of `custom` data from plugin to plugin, but generally are not expected to. All `custom` fields that were passed to `sendTransfer` MUST be passed to the transfer handler by the plugin on the receiving side. The only exception are properties which start with the underscore character (`_`), which MAY be consumed by the plugin and not passed on.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace: "All custom fields that were passed to sendTransfer MUST be passed to the transfer handler by the plugin on the receiving side"

With: "All custom fields that were set in FulfillmentInfo#custom on the receiving side MUST be set in FulfillmentInfo#custom when it is resolved by the promise on the sending side."

@adrianhopebailie
Copy link
Collaborator

If the ILP destination address starts with the ledger prefix, they should forward the transfer to the account mentioned in the destination address (usually the first segment after the ledger prefix). If it doesn't, they should forward the payment to the default connector.

I think this could be restated as:

If the ILP destination address is the address of the receiver, they should forward the transfer to the receiver's account. If it isn't, the connector should attempt to route the payment via a new outgoing transfer in the direction of the destination ILP address.

@adrianhopebailie
Copy link
Collaborator

p.s. I think the build fail is because you have used folder xxxx- and it's matching on (\d\d\d\d)

@michielbdejong
Copy link
Contributor

the plugin should expose a local HTTP server and the connector would call it?

you can short-circuit that if it's within the same process. Instead of calling plugin.listen(LOCALHOST_PORT) and the connector then using node-fetch or superagent, the connector can call plugin.handleRequest directly

Copy link
Member

@emschwartz emschwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of comments but overall I like this direction


| | Name |
|:--|:--|
| `static` | [**lpiVersion**](#version) = 2 |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: "lpi" is indistinguishable from "Ipi" in many fonts, what about just calling this version?


Emitted whenever a connection is successfully established.

#### Event: `disconnect`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ledger plugins try to reconnect themselves or should that be up to the user of the plugin? If the plugin will try to reconnect itself, what's the point of having this event?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ledger plugins try to reconnect themselves or should that be up to the user of the plugin?

-1 - That seems like functionality that should be written once in whatever consumes the plugin.

Also, a plugin CAN try to reconnect silently if it is able to buffer requests in the interim. That would be a feature of a good plugin but seems like a high bar for all plugins.

Finally, that makes some assumptions about the underlying transport that will be used which I think is a mistake.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the connector listens to the disconnect event to know when it should stop advertising a certain route. In fact, it actively informs peers that it can no longer service this route.

For this use case alone, I think this is an important feature.


For a detailed description of these properties, please see [`LedgerInfo`](#class-ledgerinfo).

#### Event: `connect`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just call connect and wait for the promise to resolve instead of having a separate event for this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's useful for monitoring to have events even if they are just pub-sub type notifications. i.e. You likely won't react to them other than to log them.

I was tempted to suggest we should still keep the transfer and fulfillment events for this reason

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use @emschwartz' idea of middleware to serve the transfer events use case. That's what convinced me to take them out.

class EventMiddleware extends EventEmitter
{
  constructor (next) {
    super()
    this.next = next
  }

  getTransferHandler() {
    return async (transfer) => {
      this.emit('prepare', transfer)

      try {
        const fulfillInfo = await this.next(transfer)
        this.emit('fulfill', transfer, fulfillInfo)
      } catch (err) {
        this.emit('reject', transfer, err)
      }
    }
  }
}

And you could hook that into sendTransfer to get events for outgoing transfers and info registerTransferHandler for incoming transfers.

const incomingEvents = new EventMiddleware(myActualTransferHandler)
plugin.registerTransferHandler(incomingEvents.getTransferHandler())

incomingEvents.on('prepare', (transfer) => {
  console.log('An incoming transfer for %s was prepared', transfer.amount)
})

| [**InvalidFieldsError**]() | Arguments or configuration were invalidated client-side |
| [**UnreachableError**]() | An error occured due to connection failure |
| [**NotAcceptedError**]() | An operation has been rejected due to ledger-side logic |
| [**NoSubscriptionsError**]() | A transfer cannot be delivered because there are no active websockets |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between NoSubscriptionsError and UnreachableError?


Initiates an account-local transfer. A transfer MUST contain an `amount` of zero or more and MAY have attached `data`. See the description of the [Transfer](#class-transfer) class below.

All plugins MUST support amounts in a range from zero to some maximum. Plugins MAY implement zero-amount transfers differently than other transfers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a recommended maximum? Should it fit into a UInt64?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't seem like we need a maximum. We just need an error that tells you what the maximum is.

A connector should:

try {
   plugin.sendTransfer(...)
} catch (err) {
  if (err.name === 'MaxAmountExceeded') {
    // Get maximum amount from err.maxAmount and trigger a
    // "payment too large" ILP error to tell the sender to send a
    // smaller amount.
  }
}

'2017-12-02T11:51:26.627Z'
```

#### Transfer#custom
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like custom is either for the ledger plugin itself, or for the next connector, whereas the data is end-to-end data that should be treated as opaque by the intermediaries and must be passed on.


Fulfillments are base64url-encoded values with a length of exactly 32 bytes.

Ledger plugins that do not support holds MUST reject with an `HoldsNotSupportedError` if this parameter is provided.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be removed


A cryptographic fulfillment that is the SHA-256 preimage of the hash provided as the [`executionCondition`](#transferexecutioncondition) when the transfer was first prepared.

Fulfillments are base64url-encoded values with a length of exactly 32 bytes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make it a buffer?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are fixed length. Isn't a Buffer uneccessary over-head.

```js
{
// Store a value under a key
put: (key, value) => {
Copy link
Member

@emschwartz emschwartz Dec 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the value assumed to be JSON? Could you store a Buffer instead (that's what the LevelUP API assumes unless you're using the encoding-down wrapper)?

#### ConnectOptions#timeout
<code>**timeout**:Number</code>

The number of milliseconds that the plugin should spend trying to connect before giving up.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this just be handled by the caller (with a Promise.race)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how would you tell the plugin to stop trying to connect? Call disconnect?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ironically, I just ran into an issue in ilp that's very similar to this. ilp uses Promise.race to timeout connecting, but Promise.race doesn't do anything to stop the thing that "lost" the race.

In my case what this did is that the connection timeout was keeping the tests from exiting. (Mocha 4 no longer force-exits unlike Mocha 3, so you have to clean everything up properly.)

I did a quick fix by setting a shorter timeout. But imo, the proper fix is to remove Promise.race and replace it with setTimeout and call clearTimeout once the connection completes successfully. If you really wanted to use promises, you could use bluebird and it's idea of cancellation, but I think that's actually less clean.

General lesson: When using Promise.race, be aware that the thing that loses the race may keep doing stuff that's no longer necessary and/or that you don't want it to be doing.

@justmoon
Copy link
Member Author

justmoon commented Dec 4, 2017

p.s. I think the build fail is because you have used folder xxxx- and it's matching on (\d\d\d\d)

Yes, I'm aware. I'll assign an RFC number when we're closer to being ready to merge.

@adrianhopebailie
Copy link
Collaborator

Yes, I'm aware. I'll assign an RFC number when we're closer to being ready to merge.

What I meant is that you could use 0000- until you take a number

| `String` | [ilpRejection.triggeredBy](#interledgerrejectionerrorilprejection) | ILP address from which the rejection originates |
| `String[]` | [ilpRejection.forwardedBy](#interledgerrejectionerrorilprejection) | The ILP addresses of zero or more connectors who forwarded the rejection |
| `String` | [ilpRejection.triggeredAt](#interledgerrejectionerrorilprejection) | ISO 8601 datetime. Describes when the rejection originally occurred. |
| `Object` | [ilpRejection.additionalInfo](#interledgerrejectionerrorilprejection) | Additional details about the error |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need more details on what value types are allowed in here and how you're supposed to use it. Do we only support JavaScript JSON values? What's the range on supported numbers?

Also, is every implementation free to use its own key names? Can connectors modify this to add more details? What if they want to add a property but the key is already used by another application?

Overall, I'm not sure that having an object here is much more helpful than having a single data property that transport protocols can define the encoding for, or is otherwise assumed to be a string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a data property that is specifically for Transport Layer protocols and then specifying that additionalInfo can contain key-value pairs as UTF-8 strings?

Copy link
Contributor

@michielbdejong michielbdejong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's useful to have field names that differ from the 4 headers used in http-over-ilp (not to be confused with http-ilp).

The mapping is as follows, only the field names are different:

expiresAt -> ILP-Expiry
amount -> ILP-Amount
destination -> ILP-Destination
executionCondition -> ILP-Condition

We could also keep the names proposed here, but then I would propose changing them in #349.


The `Transfer` class is used to describe transfers from the originator of the sendTransfer call towards some `destination`. All fields are required and MUST NOT be undefined. However, `amount` MAY be the value `'0'`, `data` MAY be an empty buffer and `custom` MAY be an empty object.

###### Fields
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the light of #349 I would change this to:

{
  headers: {
    'ILP-Destination': 'g.crypto.bitcoin.1XPTgDRhN8RFnzniWCddobD9iKZatrvH4.~asdf1234',
    'ILP-Condition': 'x73kz0AGyqYqhw_c5LqMhSgpcOLF3rBS8GdR52hLpB8',
    'ILP-Expiry': '2017-12-07T18:47:59.015Z',
    'ILP-Amount': '1000'
 },
 body: <body>
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe

{
  destination: 'g.crypto.bitcoin.1XPTgDRhN8RFnzniWCddobD9iKZatrvH4.~asdf1234',
  condition: 'x73kz0AGyqYqhw_c5LqMhSgpcOLF3rBS8GdR52hLpB8',
  expiry: '2017-12-07T18:47:59.015Z',
  amount: '1000'
  body: <body>
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 for the first transfer proposal, but +1 for something like the 2nd transfer proposal.

  • The prefixes (i.e., ILP-... in ILP Over HTTP #349 are there as an artifact of using HTTP headers to carry the data (to distinguish between other headers that might be arbitrarily added to the request as part of a normal and valid HTTP request). Conversely, In this interface, the prefixes are not needed because there's less of a risk of "other data fields" colliding with these fields names -- mainly because we're defining the field names here.
  • body is an artifact of HTTP. We should call it what it really is here (I like Adrian's suggestion above of peerData and pluginData), and just make new attributes for the data we need.

@justmoon
Copy link
Member Author

Regarding whether to keep the ILP packet as an opaque blob or not:

Randomly reread chapters 1 and 2 of ASN.1 Complete recently, which talk about the exact problem we've run into. Should you separate your protocol into layers (as TCP/IP does) or should you merge them into one (as most telecommunication protocols did at the time)? It's a trade-off of simplicity vs extensibility. If you layer your protocol, each layer can evolve independently. If you merge them, they become simpler (in the short run.)

It's a trade-off and therefore: Too many layers are bad. As are too few.

So the question is: Do we need the ledger layer to be able to evolve independently from the Interledger layer? Do we want to be able to think about them independently?

I would say, yes, absolutely. We already have two actively used, competing, ledger layer protocols in BTP and ILP-over-HTTP (not to be confused with HTTP-ILP) just based on preferences without our own small team. Others may wish to use protocol buffers. In the future, we may not want to have a dependency on the TCP/IP stack at all and come up with a ledger protocol that layers straight on a secure communication link.

Merging the Interledger and ledger layers seems simple, because you're getting rid of a layer, but it isn't because now it's harder to evolve your ledger layer or your Interledger layer independently since they're tied together.

If we agree that we want to have separation between the ledger/account layer and the Interledger layer, then we need to decide whether the interface is:

  1. You need to transfer this small set of fields (Destination, Data, Error-Code, Error-Triggered-By, ...) or
  2. You need to transfer an opaque blob (ILP packet)

The second provides a lot less opportunity for ledger protocols to get it wrong and allows new ILP packet types to be created in the future, without having to modify each ledger protocol. (Note that you wouldn't just want to update the implementations, you'd want to update the protocols.)

We (at least some of us) have collectively come to this conclusion before. That's where the ILP packet came from. So perhaps it's not surprising that thinking about it again would lead back to the same place.

So, considering this, I would strongly advocate we make the LPIv2 interface such that preparation, fulfillment and rejection each take/emit an ILP Buffer, which is treated as an opaque blob by the plugin.

@emschwartz emschwartz mentioned this pull request Dec 11, 2017
@adrianhopebailie
Copy link
Collaborator

Personally I am in favor of us leaning more toward simplicity in this case.

I agree that the "correct" way to layer these protocols is to have distinct layers between the ledger and ILP layers but I think the cost is higher than we estimate and that the extensibility requirement is overstated.

The interface between the Ledger and ILP layers is unique (compared to other inter-layer interfaces) because the majority of the data elements are standardized by the Interledger standard yet in the current model they are passed both at the ledger and ILP layers.

Ledger layer:

  • Condition
  • Expiry
  • Fulfillment

ILP layer:

  • ILP Address
  • ILP Error Code/Name
  • ILP Error Source (and intermediaries)
  • ILP Error Timestamp

I think there is a strong argument that these should all be at the same layer.

I have made the case previously that the Condition and Fulfillment should be in the ILP layer and I think we have agreed that this is theoretically correct but practically makes no sense because they will be mostly used at the ledger layer. We compromised for simplicity.

In my opinion the same argument could be made for moving the other fields to the ledger layer.

There are a lot of benefits:

  1. No need to define a new packet format and encoding.
  2. The end-to-end data is just transported as the payload of whatever Application Communication Protocol is used for the ledger layer protocol (eg: HTTP, CoAP, JSON-RPC, custom).
  3. We can leverage existing methods for securing and integrity-checking this data end-to-end (otherwise they are only usable at the ledger layer)
  4. No need for a fulfillment packet that is just an envelope around a BLOB
  5. No need for a payment packet that simply adds an address to the data

In summary, re-use a lot of what already exists and invent less ourselves making implementation and understanding simpler.

I find it very difficult to justify the complexity we have currently to cater for extensibility that seems very unlikely to ever be required.

@sappenin
Copy link
Contributor

I concur 100% with @adrianhopebailie -- the benefits he outlined are really compelling (I had a bunch of feedback, but I think Adrian captured nearly all of it).

Only thing I would underscore is developer adoption -- implementing a packet with codecs in different languages might seem trivial, but it is a significant barrier to adoption, IMHO, and it's not clear that we couldn't accomplish the same things using HTTP and a well defined spec.

@dappelt
Copy link

dappelt commented Dec 14, 2017

On a different note: If getBalance is removed, how does a user know what his balance is? Or is the idea that the user is not aware of his balance at all?

Knowing how much funds a user has available is essential for many purchase decisions. For example, let's say a user has 5€ left in his wallet. He would like to buy a coffee and a newspaper, both cost 3€. If the user is aware of his balance, he would know that he can afford either the coffee or the newspaper, but not both, and make a conscious purchase decision. On the other hand, if the balance is unknown to the user, he would buy one item and only when he tries to pay for the second item he would figure out that he has not enough funds for both.

Copy link
Contributor

@michielbdejong michielbdejong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing plugin.sendRequest only makes sense if we pivot to pennies, if we want Interledger to keep supporting the slightly wider variety of ledgers which it supports now, then the choice between on-ledger messaging and side-channel messaging is a plugin-level decision, and then IMHO we should keep that method as it is.

#### LedgerPlugin#registerTransferHandler
<code>ledgerPlugin.registerTransferHandler( **transferHandler**: ( transfer: [Transfer](#class-transfer) ) ⇒ Promise&lt;[FulfillmentInfo](#class-fulfillmentinfo)> ) ⇒ null</code>

Set the callback which is used to handle incoming prepared transfers. The callback should expect one parameter (the [Transfer](#class-transfer)) and return a promise for the resulting [FulfillmentInfo](#class-fulfillmentinfo). If the transfer is rejected or an error occurs, the callback should reject the transfer. In general, the callback should behave as [`sendTransfer`](#ledgerpluginsendtransfer) does.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justmoon While working on the PSKv2 implementation using the LPI2 I realized we also want to have a way to ignore an incoming transfer. This is necessary if you have two receivers listening on one account and want to make sure they don't interfere with one another.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually @justmoon and I were briefly discussing this yesterday. It might be a best practice to have each listener get its own sub-account. This means your ILP address would change every time you reconnect (which is OK because they're ephemeral anyways), but all your listeners would still have a shared balance. The ILP-DCP protocol would just have to take into account which listener is asking, and then send out to that same listener when notifications come in.

@michielbdejong
Copy link
Contributor

I think we will eventually want to add the to field back as an optional parameter on sendData and sendMoney so that you can address multiple peers on one ledger. If you would split it into one plugin per peer, then their balances would go up and down in unison, which would be less helpful than if we just have one plugin per ledger, with a to field. After all, this is still called the ledger plugin interface, not the Peer Interface. But for now I guess it's fine to remove to, because for the forseeable future, we will mainly be working with one-to-one relationships, not one-to-many.

| | [**registerTransferHandler**](#ledgerpluginregistertransferhandler) ( transferHandler ) <code>⇒ null</code> |
| | [**deregisterTransferHandler**](#ledgerpluginderegistertransferhandler) ( ) <code>⇒ null</code> |
| | [**sendData**](#ledgerpluginsenddata) ( data ) <code>⇒ Promise.&lt;Buffer></code> |
| | [**sendMoney**](#ledgerpluginsendmoney) ( amount ) <code>⇒ Promise.&lt;Buffer></code> |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: sendMoney or sendValue?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sendValue would be a confusing name, because it sounds like it takes a value and communicates that value to your counterparty. sendMoney is technically not correct in all circumstances, but I think it makes more sense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sendAsset?


### Sending

#### LedgerPlugin#sendData
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you specifically wanted to implement a ledger plugin that did send ILP packets over conditional ledger transfers, would you just implement one that looks at the data being sent and handles it in a different way?

@michielbdejong
Copy link
Contributor

michielbdejong commented Jan 5, 2018

I think it would be good to clarify two things:

  • how do sendData and sendMoney affect the account's balance
  • how can you use on-ledger transfers or paychan claim updates to rebalance?

For example, think about how this would work in ilp-plugin-xrp-paychan:

Both parties open a payment channel to the other party, but don't send any claims yet. Balances start at zero.

Using plugin.sendData, the Sender sends a Prepare packet for 10 drops, and the Receiver fulfills it, so now the Receiver's balance is 10, and the Sender's balance is -10. Right? This is the unsecured balance.

Now in order not to hit their peer's maxUnsecuredBalance, the sender signs a paychan claim for 10 drops. How can they send this claim to their peer, and make their own balance go back up to 0?

Option A) the Sender calls plugin.sendMoney with a negative amount, and attaches the claim
Option B) the Sender sends the claim out-of-band (it used to be in the Fulfill Response), and then the Receiver calls plugin.sendMoney with a positive amount
Option C) instead of plugin.sendMoney, we define plugin.claimMoney, such that plugin.claimMoney with a positive amount is equivalent to plugin.sendMoney with a negative amount.

I think maybe there's a confusion about what we consider the "balance" of each peer? In any case, it would be good to clarify this - the word "balance" does not occur in the text now.

@sharafian
Copy link

sharafian commented Jan 5, 2018

Using plugin.sendData, the Sender sends a Prepare packet for 10 drops, and the Receiver fulfills it, so now the Receiver's balance is 10, and the Sender's balance is -10. Right? This is the unsecured balance.

Now in order not to hit their peer's maxUnsecuredBalance, the sender signs a paychan claim for 10 drops. How can they send this claim to their peer, and make their own balance go back up to 0?

I would look at this a different way. When the sender calls sendData, they are giving nothing to their peer (a sendData call carries no money), but the peer is forwarding an interledger payment on their behalf, which they will later have to settle. This means the peer is out 10 units. This is what brings the sender's balance to -10. They've effectively fronted you the money.

Now when the sender wants to pay off their debt (i.e. bring the balance back up), they call sendMoney. This call actually transmits 10 units of money to the peer, and the peer keeps this settlement without forwarding anything out. This means the peer has gained 10 units. This brings the sender's balance back to 0.

TL;DR for other people following this: sendData increases debt because your peer gets nothing in and sends a payment out for you. sendMoney decreases debt because the peer gets money in and sends nothing out for you.

@michielbdejong
Copy link
Contributor

Right, I see your point. So then I agree, for the LPI the naming does makes sense; we just say that plugin.sendData decreases the sender's trustline balance, and plugin.sendMoney decreases the sender's on-ledger or paychan-secured balance, so that both can have positive amounts and it still makes sense. So then my only issue is with the way the word "transfer" is used in #360.

justmoon and others added 11 commits January 20, 2018 08:22
This information will now move to the connector configuration. If we
want `ilp` client to auto-configure, we can create a participant-level
protocol like IL-DCP.
Removed all special error definitions. Interledger rejections are now
just successful data responses. We no longer need to react to different
exceptions differently, therefore the spec does not need to specify what
errors are possible anymore.

Removed Transfer, FulfillmentInfo classes.
Undefined is more natural to use. E.g. `return` instead of `return
null` and `Promise.resolve()` instead of `Promise.resolve(null)`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
7 participants