-
Notifications
You must be signed in to change notification settings - Fork 51
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
DIP-1 becomes purely about off-chain #148
Conversation
* Move Travel rule into an independent DIP-13 * Add an optional result field to responses * Improve statements around cid on responses * Use must and instead of should around UUID requirements * Add X-DIEM-ADDRESS as required -- this seems ... how did this get added to the specification as required? * Removed example code -- should be in a guide not in a spec * Introduce a PingCommand * Make author's list inclusive
|
||
| error | [OffChainErrorObject](#offchainerrorobject) | N | Details of the error when status == "failure". | | ||
| result | Object | N | An optional JSON object that may be defined when status == "success". | | ||
| cid | str | N | The Command identifier to which this is a response. Must be a UUID according to [RFC4122](https://tools.ietf.org/html/rfc4122) with "-"'s included and must match the 'cid' of the CommandRequestObject. This field must be set unless the request to which this is responding is unparseable. | |
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.
Other cases:
- No cid in the request
- cid in the request is not a valid uuid
- Blob is valid json, but there is a field type does not match (for example: command_type value is a int), causes the deserialization failure. You probably can still parse out the cid by not deserializing json blob in this case, but need call out and handle it probably.
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 all of these imply that the request is unparseable
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.
What you mean unparsable? The request json string is parsed successfully for all above cases.
|
||
**Reference ID**: Every Object type contains a `reference_id` field which is a unique reference ID for the Object. The reference ID is always specified by the payment initiator VASP (the VASP which originally created the Object). This value should be globally unique. It is recommended to use a 128 bit long UUID according to [RFC4122](https://tools.ietf.org/html/rfc4122) with "-"'s included. | ||
Each VASP exposes an HTTPS POST end point at `https://<hostname>:<port>/<protocol_version>/command`. The protocol_version is v2 for this iteration of the off-chain APIs. |
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.
Current version need bump up to "v3"
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 thought the intent of some of these changes would be to make it be done in a compatible way so that you don't need to bump the version number. If we're bumping the version number then we can and should make a lot more changes. Since going from v2 -> v3 is a "breaking" 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.
Why are we bumping it to v3? This doesn't contain any breaking changes with the current protocol. Which is why I'm not sure why we included new mandatory fields in the specification doc ;)
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 bump up version for the change added.
I feel confusing when doc has v3, but the protocol version is still v2.
But up to you.
strip out X-DIEM-ADDRESS and make it clear that base_url does not support multiplexing
DIP-1 is already in Final stage. Please note partners have implemented or in the process of implementing DIP-1 as it stands today. Changing the Spec which is already final can not only create confusion but may introduce compatibility issues in production. Would the right process be to start new DIP and then deprecate DIP-1? |
What makes you feel like this is an incompatible update to the dip? It's largely improving the readability and improving the organization. If it breaks anything or creates any incompatibilities, then the update has failed it's purpose. With respect to a new dip, the intent here being a forward compatible update, we've agreed to using versioning. If we were to introduce a completely different off chain format, say ethereum whisper, it would be a different dip. |
While I do appreciate that incompatible updates to a protocol are not as disruptive, I think this DIP has gone substantive changes over time and we should be following the process of DIP deprecation rather than updating it. Also as this is an optional standard, it makes sense to keep it in a draft stage until we have sufficient confidence to more to Accepted/Final (this would avoid multiple DIP deprecation/updates). From https://dip.diem.com/overview |
I think this highlights a few issues with the DIP process as a whole. Specifically that DIPs cannot be looked at as a cohesive Specification. In particular it might be more ideal to have a single offchain Specification (that includes everything) and instead use the DIP process as a way to propose and ratify changes to said specification. |
``` | ||
{ | ||
"_ObjectType": "CommandResponseObject", | ||
"result": {}, |
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.
We may remove result if it is empty
|
||
**Reference ID**: Every Object type contains a `reference_id` field which is a unique reference ID for the Object. The reference ID is always specified by the payment initiator VASP (the VASP which originally created the Object). This value should be globally unique. It is recommended to use a 128 bit long UUID according to [RFC4122](https://tools.ietf.org/html/rfc4122) with "-"'s included. | ||
Each VASP exposes an HTTPS POST end point at `https://<hostname>:<port>/<protocol_version>/command`. The protocol_version is v2 for this iteration of the off-chain APIs. |
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 bump up version for the change added.
I feel confusing when doc has v3, but the protocol version is still v2.
But up to you.
At this point, the plan is to take this content and leverage it in a spec instead of updating the DIP. Once the spec is fully written, we should eliminate referring to DIPs from our nomenclature and instead refer to the spec. |
Action items: