Open adrianhopebailie opened 5 years ago
Some comments:
I'm concerned about the use of the Payee.PartyIdInfo.PartySubIdOrType field to hold the ILP address of the payee (I assume that it is the item in the Transfer object that is meant, though it says "header"; or have I misunderstood?) The specification shows examples of how this item might be used for other purposes (see Section 5.2,) and these use cases would be impossible if we were to insist that the SubIdOrType field were to contain the ILP address of the party. In my view, it would be better to separate the two purposes from each other, and store the ILP address in a separate subfield of the PartyIdInfo structure (as it might be, an optional field called Payee.PartyIdInfo.PartyIlpAddress.)
I also agree with Henrik: this field should have an appropriate maximum length.
@MichaelJBRichards Using a separate optional element instead of PartySubIdOrType sounds good, would make it easier to explain in the documentation and therefore also easier to understand.
@ehenrka and @MichaelJBRichards I have addressed some of your comments by editing the issue.
@ehenrka asked:
In the earlier change request in Word you mentioned that the payload (the transaction) should be base64 encoded, is that still valid? Does not seem like that based on the above. Could you please clarify?
There is a separate conversation going on about the need for this. The primary requirement is for a piece of data that can be the input to the condition generation algorithm that is the same in both the quote and transaction. This allows the payee to return some data that is sent back in the transaction and can be used to regenerate the fulfilment.
I have attempted to address this in #14
asked you earlier to look at sending back data in the callback PUT /transfers, the data that is currently sent as part of PUT /transactions. The current GET /transactions request was added to the API as Interledger v1 did not support sending back data together with the fulfilment in the PUT /transfers. This seems to be supported in ILP v4 now (https://interledger.org/rfcs/0027-interledger-protocol-4/, ILP Fulfill), is my understanding correct? Can we please include the data in PUT /transfers instead, so that the /transactions resource can be deprecated?
Thanks for raising this again.
I hadn't anticipated that you'd want to deprecate the /transactions
API completely.
In the callback for /transactions
the following fields are present:
Name | Cardinality | Type | Description |
---|---|---|---|
completedTimestamp | 0..1 | DateTime | Time and date when the transaction was completed. |
transactionState | 1 | TransactionState | State of the transaction. |
code | 0..1 | Code | Optional redemption information provided to Payer after transaction has been completed. |
extensionList | 0..1 | ExtensionList | Optional extension, specific to deployment. |
Do you anticipate the /transfer
callback containing a new field (e.g.transactionResponse
) that contains this data?
Thanks @adrianhopebailie.
I will have a look at #14.
The GET /transactions and related PUT /transactions was only added because ILP did not support transferring any data in the transfer callback (PUT /transfers). We had a requirement to be able to return a OTP code for some use cases, for example in a vending machine where you could input the code after the transaction was completed. If the code can be returned as part of PUT /transfers instead, then I do not see any purpose of keeping the /transactions resource. The parameters completedTimestamp and extensionList are already part of PUT /transfers, transactionState should not differ from transferState more than by the state names. It would simplify the API to remove a resource which is not really needed. But maybe other members of the CCB would like to keep it?
@ehenrka are some of these fields required for both the transfer
and transaction
in the case where the transaction may consist of multiple transfers?
Maybe we should use the same pattern as the request and put the transaction result inside the transfer?
Name | Cardinality | Type | Description |
---|---|---|---|
fulfilment | 0..1 | IlpFulfilment | Fulfilment of the condition specified with the transaction. Mandatory if transfer has completed successfully. |
transactionResult | 0..1 | TransactionResult | The result of the end-to-end transaction. |
completedTimestamp | 0..1 | DateTime | Time and date when the transfer was completed. |
transferState | 1 | TransferState | State of the transfer. |
extensionList | 0..1 | ExtensionList | Optional extension, specific to deployment. |
Where TransactionResult
is:
Name | Cardinality | Type | Description |
---|---|---|---|
completedTimestamp | 0..1 | DateTime | Time and date when the transaction was completed. |
transactionState | 1 | TransactionState | State of the transaction. |
code | 0..1 | Code | Optional redemption information provided to Payer after transaction has been completed. |
extensionList | 0..1 | ExtensionList | Optional extension, specific to deployment. |
Name | Cardinality | Type | Description |
---|---|---|---|
fulfilment | 0..1 | IlpFulfilment | Fulfilment of the condition specified with the transaction. Mandatory if transfer has completed successfully. |
code | 0..1 | Code | Optional redemption information provided to Payer after transaction has been completed. |
completedTimestamp | 0..1 | DateTime | Time and date when the transfer was completed. |
transferState | 1 | TransferState | State of the transfer. |
extensionList | 0..1 | ExtensionList | Optional extension, specific to deployment. |
Thanks @adrianhopebailie. There is no explicit support in the API for having multiple transfers for one single transaction today. I have not heard any requirements for it either, as all transactions are low-value transactions anyways (at least for now).
Proposal 1 is clean to reuse the same pattern as in POST /transfers, and reuse the current PUT /transactions data model, as well as more future proof if more elements are required to be added in the future to avoid cluttering the PUT /transfers data model.
(In reality I don't think we need the completedTimestamp
in both PUT /transfers and the TransactionResult
, it should be exactly the same. Additionally transactionState
in the TransactionResult
should be the same as for the transfer (the transferState
) but with another name. If the transfer is successful (COMMITTED) then the state of the transaction must be COMPLETED. If the transfer failed for some reason, then there should be no TransactionResult
. But for backwards compatility we can keep it like the proposal unless someone insists to clean up).
There is no explicit support in the API for having multiple transfers for one single transaction today. I have not heard any requirements for it either, as all transactions are low-value transactions anyways (at least for now).
This is the whole purpose of having Interledger addresses in the transfers so that they can form part of a chain of transfers addresses to entities that may be in other networks.
We have already built two POCs to demonstrate this:
I also prefer proposal 1 but suggest we review the design of TransactionResult
as you suggest.
@adrianhopebailie Sorry for the confusion, I thought you meant multiple lower-value transfers to the Payee which are then combined into one transaction, not multiple transfers along the way through different networks to the Payee which are needed for cross-border and cross-currency.
NOTE: As discussed on the CCB call on 11 June we plan to close this issue on 18 June if there are no further comments.
Unless I misunderstand something, the change in https://github.com/interledger/rfcs/commit/3b280591d7cc0840f7d2a48846208ce22d8da00e changes this CR completely, as it seems like we still need the ILP Packet to be encoded to follow ILPv4 correctly?
Please correct me if I'm wrong..
The current API (v1) is aligned with a legacy version of the Interledger protocol (v1). We should align the API to ILPv4, not only for the sake of deprecating what is an outdated implementation, but also to enable us to use the latest open source Interledger components.
FSPIOP API as a "Bilateral Protocol" in ILP
In an Interledger transaction ILP packets are passed between participants with a direct bilateral relationship. The ILP packet headers carry the details of the transfer from the one participant to the other whereas payload of the packet are end-to-end data that is used to convey transaction information from the payer to the payee.
When looking at the API, in the context of the Interledger protocol, we consider a transfer between two DFSPs as analogous to the exchange of ILP packets between nodes. i.e. The
/transfer
API represents a single "hop".The end-to-end data is carried in the
Transaction
object (currently encoded inside the ILPv1 packet payload)In ILPv1, the ILP packet was enveloped in what was called a "ledger protocol" message. Ledger protocols were used between nodes and their shared ledger and contained the details of the transfer including the condition, expiry and transfer amount. The ILPv1 packet contained the
ILP address
of the payee, thedestination amount
and the end-to-enddata
payload.In ILPv4, the protocol was adjusted to be used directly between nodes via a "bilateral protocol". In the bilateral protocol model the detail of the transfer between the the two nodes is now included in the ILPv4 packet headers, including:
transfer amount
execution condition
transfer expiry
The
destination amount
was dropped from the packet headers as this is sensitive data and is expected to be part of the end-to-end payload.In Interledger terminology the FSPIOP
/transfer
API is therefor a bilateral protocol (carrying the details of the transfer between nodes), however it is missing the ILP address of the payee.The concrete changes proposed are:
ilpPacket
from thetransfer
object.Transaction
object as a property of thetransfer
object (previously the payload of the embedded packet).Payee.PartyAddress
. This data element will be an ASCII string with a length restriction of 1023 characters.With these changes the
/transfer
API is now a direct mapping from the ILPv4 packet:/transfer
APItransfer.payee.address
transfer.amount
transfer.expiry
transfer.condition
transfer.transaction