-
Notifications
You must be signed in to change notification settings - Fork 111
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
feat: introduce best-effort payments (with no amount specified) #323
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justmoon mentioned the idea that this might be a good time to add a Time to Live value. I don't feel too strongly about it either way, but adding a new packet type is a decent "speak now (for new features/fields) or forever hold your peace" moment. So...thoughts on TTL (or other new fields)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if signed data is expected to come back from the receiver, the payment packet could contain the pubkey of the receiver. Even if the ledgers don't enforce a crypto-condition (just like there might be ledgers in the chain that don't enforce the hashlock), it would be correct to define the pubkey and the signature scheme as an expectation of the payment as a whole
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't sound like a good idea to me. If some parties enforce it but not all, that would be easily exploitable by sending payments you know one party will accept that the other won't. I think whether or not we have fulfillment data is orthogonal to this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A ledger sits between two connectors. The sending connector should make sure they know if the next ledger enforces it or not. If they know it doesn't, then they need to trust the next connector for the in-flight amount (because the next connector could claim the in-flight amount without producing correctly signed fulfillment data). But that's just a basic HTLA trade-off. You cannot exploit a remote connector, you can only get into disputes with your direct neighbors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a jankier way of doing Crypto Conditions that would run into similar problems. Unless you can rely on the whole network supporting a feature (that needs the whole path to support it in order for it to work), it either can't really be used or it becomes a point of incompatibility between different parts of the network.
There was a problem hiding this comment.
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 about what you're arguing for here and how this is related to this discussion (as opposed to #314). Are you proposing that a public key would go in the Interledger packet and that a signature by the corresponding private key would be an additional condition for executing transfers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, i agree it would be too much work to add such enforcement. i just feel that it should be clear, if Alice sends Bob a conditional transfer, and Bob produces the fulfillment but nothing else, will Bob have done his job? Or does his job also include delivering the attached data? Or is this something neither Alice nor Bob can know by just looking at the packet? If Alice includes the final receiver's pubkey in her ask to Bob, and Bob comes back with only a fulfillment, then it will be clear that Bob didn't do all the things Alice hoped he would do.
But none of this is necessary anyway if we decide against adding data along with the fulfillment as part of Interledger, and decide that that data will just have to travel out-of-band (even if it piggybacks over side-protocols)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emschwartz @justmoon what would be the purpose of the TTL and how would it be implemented by senders/connectors/receivers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's the question under discussion in #314. I think it should either be an expectation of the ILP protocol that connectors send response data back, or not. I don't think there needs to be an indication on the forward path whether it's expected. The transport protocols will know when it's expected or not.
There would be a field in the packet that the sender would set (to a number less than 256 and probably closer to 10). Each connector would decrease that value by 1 before passing on the packet. The purpose, like in IP, is to mitigate the damage caused by routing loops. When the value gets to 0, connectors would not forward the packet any more and would reject the incoming transfer with a specific error.
Previously we've thought that we didn't need a TTL or max number of hops because the routing loops would be implicitly handled by the amount and/or transfer timeout. For packets with no amount in them, it's less clear to connectors when there is not enough money to deliver the destination amount -- unless the transfer amount is 0. @justmoon made the point that relying on the amount or transfer timeout may take too long for packets to get dropped for them to mitigate the damage of routing loops.
I'm on the fence about whether it's worth adding a max number of hops. Leaning towards no right now but could be convinced otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. What if we add and make
FF
or something a flag to ignore so it can be optional.