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

docs: add settlement engine RFC #536

Merged
merged 35 commits into from
Oct 20, 2019
Merged

docs: add settlement engine RFC #536

merged 35 commits into from
Oct 20, 2019

Conversation

kincaidoneil
Copy link
Member

This standardizes an agreed API between @emschwartz, @adrianhopebailie, @matdehaast, @sappenin and I, with some minor changes I am proposing from the existing Swagger definition.

Areas that may need improvement:

  • Explanation of how accounting works
  • Clarity around how implementations should handle idempotency and undoing balance updates
  • Not all the HTTP endpoints are nicely documented yet (but everything can still be included in an embedded Swagger definition)

Thoughts? What's unclear or needs better explanation? What's still "unaccounted" for? (no pun intended...)

@gakonst
Copy link
Member

gakonst commented Jun 20, 2019

Regarding the asset scale, I do not see a clear benefit by making it configurable on a per request basis. IMO it should be a parameter on the settlement engine and the node's account.

Also, I'd expect that it's set to the amount of decimals of the currency, or some multiple of that such that you are able to make payments which are tinier than the minimum denomination (cf sub-sat payments in lightning)

e.g my account is configured to run with 9 decimals (1 satoshi), but the settlement engine is configured with 18. As such, I'd need to send 1e9 units (which would make 1 sat) before settlement happens

Copy link

@sharafian sharafian left a comment

Choose a reason for hiding this comment

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

This looks like a good start. I like the protocol itself, but I think the explanation could still use some work, especially because this specification is one of the longer ones.

0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
@adrianhopebailie
Copy link
Collaborator

Regarding the asset scale, I do not see a clear benefit by making it configurable on a per request basis. IMO it should be a parameter on the settlement engine and the node's account.

If scale was configured you'd need a way for the SE to be made aware of the scale used by the connector and the connector to be made aware of the scale used by the SE.

This implies either:

  1. some handshake between the systems to establish each other's config
  2. the operator configures each system with the settings of the other system and we hope they are never out of sync

IMO, it's simpler to just put the scale in the API calls

0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
@kincaidoneil
Copy link
Member Author

kincaidoneil commented Jul 10, 2019

Added an initial set of edits putting it in the context of Interledger and explaining how the accounting operates.

All comments regarding content prior to the "Settlement Engine HTTP API" heading hopefully should be addressed; most things after that are yet-to-be addressed.

0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
- **Accounts receivable**, the amount owed to the operator by its counterparty for incoming obligations the operator has fulfilled.
- Positive amount indicates its counterparty is indebted to the operator (an _asset_ to the operator).
- Negative amount indicates its counterparty has prefunded.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused here.

Why do we need 4 descriptions for these?
I think we could express the relationship between 2 nodes with either:

  1. 2 positive values
    • positive payable value
    • positive receivable value
  2. 1 positive/negative value
    • positive = receivable value
    • negative = payable value

I'm sorry if I have misunderstood.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question!

I'd highly recommend implementing it with these 2 values vs 1 single balance, but my goal here is less to explain how to implement it and much more to explain the best model to understand the accounting.

Part of the reason for laying it out this way is asking, how would an accountant talk about this interaction? What terms would they use?

On an implementation level, there are a few reasons to prefer this approach:

  • A single value doesn't differentiate between a peer owing you money for packets you've fulfilled, or you owing the peer money for packets they've fulfilled, so you're losing information that might be useful. As an example, in Switch, logic around "does the user have enough capacity to receive this payment?" would take into account how much credit has already been extended from the receivable balance, which wouldn't be possible if there was a single balance. (Not saying we need to support that use case, but might be helpful as an example where this might be useful).
  • Solves issues with prefunding: if it's a single balance, because there's way to differentiate between the two, sometimes money you prefund to a connector would cause the connector to trigger a settlement back to you, which is bad.

Copy link
Member

Choose a reason for hiding this comment

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

"does the user have enough capacity to receive this payment?"

Do we need capacity to receive money?

Also, it might be better to mention some cases that we might be able to utilize "prefunding".

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need 4 descriptions for these?
I'd highly recommend implementing it with these 2 values vs 1 single balance,

FWIW, both the Java and Rust implementations currently track balances using two different numbers:

0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
- numerous verbiage changes for clarity
- explain necessary guarantees for double-entry bookkeeping
- more explicit spec for use with ILP-over-HTTP
- rename transport/acc sys apis to "settlement engine callback api"
- rename API endpoints to match consensus/implementations
- remove confusing examples (more relevant for a tutorial)
- remove old idempotency section, new version is WIP
@matdehaast
Copy link

matdehaast commented Aug 2, 2019

@emschwartz @adrianhopebailie Is there a preference on this?

I think we should just go with PUT. Its really a nit picky issue that doesn't functionally change anything

Copy link
Contributor

@sappenin sappenin left a comment

Choose a reason for hiding this comment

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

@kincaidoneil Here are some suggestions around around clearer wording for asset units and scales.

  • Other than giving some example, we shouldn't repeat "asset or currency" all over. Instead, we should just define "asset" as being lots of things, and then use "asset" everywhere.
  • We shouldn't define anything in here in terms of "money" (assets can be any fungible thing) -- though I think it's good to actually use currency in our examples, which is what's there already.

Curious to hear what you think...I'm going to resolve my other comment on this topic.

0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
0000-settlement-engine/0000-settlement-engine.md Outdated Show resolved Hide resolved
kincaidoneil and others added 2 commits August 5, 2019 15:34
@kincaidoneil
Copy link
Member Author

Other than giving some example, we shouldn't repeat "asset or currency" all over. Instead, we should just define "asset" as being lots of things, and then use "asset" everywhere.

We shouldn't define anything in here in terms of "money" (assets can be any fungible thing) -- though I think it's good to actually use currency in our examples, which is what's there already.

Sounds good to me 👍

(I did like that "monetary unit" / "fractional monetary unit" are dictionary phrases, but agree they may not be general enough).

- fix: miscellaneous verbiage edits for clarity
- add section explaining rationale for async settlement design
- simpler expalantion for exchanging messages between SEs
- fix: no u64 limit to quantity
- add delete account endpoint
- fix anchor links to Quantity section
- response codes are now 201 to conform to REST
- creating an account is a POST with an account ID in the body
- other misc changes per comments
- max retry interval is 1 hour
- servers may purge idempotency keys 24 hours after most recent request
- clients must retry indefinitely in response to 5xx, 409, and no response errors
- 4xx errors should have no retry
- remove min max on idempotency key length
- explain why retry behavior is specced
Co-Authored-By: pgarg-ripple <51800920+pgarg-ripple@users.noreply.github.com>
0000-settlement-engines/0000-settlement-engines.md Outdated Show resolved Hide resolved
0000-settlement-engines/0000-settlement-engines.md Outdated Show resolved Hide resolved
@emschwartz
Copy link
Member

Can we merge this now? This has been open for a really long time and we'd like to be able to refer to it. All remaining discussions can happen in follow-up PRs

@kincaidoneil kincaidoneil dismissed stale reviews from emschwartz, gakonst, and sappenin October 20, 2019 01:23

Addressed comments; reviewed version is outdated.

@kincaidoneil kincaidoneil merged commit 7071dc7 into master Oct 20, 2019
@kincaidoneil kincaidoneil deleted the ko-settlement-engine branch December 17, 2019 15:44
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

Successfully merging this pull request may close these issues.

None yet

9 participants