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

Refactor API for latest suite of protocols #130

Open
justmoon opened this issue Feb 26, 2018 · 4 comments
Open

Refactor API for latest suite of protocols #130

justmoon opened this issue Feb 26, 2018 · 4 comments

Comments

@justmoon
Copy link
Contributor

The ilp module should be updated for the latest suite of protocols. It should expose a reasonable API for ILP and PSK/Paystream.

Here is a sketch of what the new API might look like:

Basic Usage: PayStream

const ilp = require('ilp')

;(async () => {
  // Connect to a PayStream, defaults to using moneyd
  const socket = await ilp.createConnection([ilp address], [shared secret])

  // Lower the paystream maximum by 1000 (minimum will be lowered if necessary)
  await socket.pay('1000')
  // Lower the paystream minimum by 2000
  socket.payUpTo('2000')
  // Increase the paystream minimum by 4000 (maximum will be increased if necessary)
  await socket.charge('4000')
  // Increase the paystream maximum by 2000
  socket.chargeUpTo('2000')
  // Write some data
  socket.write(Buffer.alloc(128))
  await socket.flush()
  // Read data
  socket.on('data', (data) => {
    console.log(data.toString('utf8'))
  })
})()

Basic Usage: Raw packet

const ilp = require('ilp')

;(async () => {
  const { fulfillment, data } = await ilp.sendPacket({
    destination: 'test.bob',
    amount: '100',
    executionCondition: '...',
    expiresAt: '...',
    data: Buffer.alloc(0)
  })
})()

Advanced Usage: Using a custom plugin

const { create as createIlp } = require('ilp')
const CustomPlugin = require('ilp-plugin-custom')

;(async () => {
  const ilp = createIlp({
    plugin: new CustomPlugin({ /* ... */ })
  })

  // ... ready to use
})()
@emschwartz
Copy link
Member

emschwartz commented Feb 28, 2018

I like where you're going with this, but I think the methods that use relative amounts on top of the socket using absolute amounts can make for some strange edge case behavior.

Some of the method names sound like they're referring to absolute amounts but your description of the behavior makes it sound like they are making relative changes.

Method Behavior Questions / Issues
pay Lower min and max balance by the specified amount, wait until the balance has reached the max What if the max is already below the current balance? Should it lower the max by the amount anyway and wait for the balance to hit the new max?
payUpTo Lower min by amount The name sounds like it would be an absolute amount, but am I right in thinking the idea would be to make it a relative change as well?
charge Raise min and max balance by the specified amount, wait until the balance has reached the min What if the min and max are negative? Also, what should happen if this is called after pay but before the balanced has reached the max set by the pay method?
chargeUpTo Raise max by amount Similar issue with the name sounding like it's relative

socket.write(Buffer.alloc(128))
await socket.flush()

I've deprioritized the project of implementing proper TCP on top of ILP. Do you think we should work on that (or work on it in the near future)?

emschwartz added a commit to emschwartz/ilp-protocol-paystream that referenced this issue Feb 28, 2018
@michielbdejong
Copy link
Contributor

// Connect to a PayStream, defaults to using moneyd
const socket = await ilp.createConnection([ilp address], [shared secret])

Maybe we want to rename both 'socket' and 'connection' to 'stream'?

// Connect to a PayStream, defaults to using moneyd
const stream = await ilp.createStream([ilp address], [shared secret])

@michielbdejong
Copy link
Contributor

michielbdejong commented Mar 23, 2018

or ilp.STREAM.create() so that ilp.STREAM can be included from a different repo than ilp.ILQP

@michielbdejong
Copy link
Contributor

Maybe we want to rename both 'socket' and 'connection' to 'stream'?

Or one connection, over which multiple streams travel.

emschwartz added a commit to emschwartz/ilp-protocol-paystream that referenced this issue Mar 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants