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

DiemID Refactor #185

Merged
merged 1 commit into from
Jun 23, 2021
Merged

DiemID Refactor #185

merged 1 commit into from
Jun 23, 2021

Conversation

sunmilee
Copy link
Contributor

@sunmilee sunmilee commented Jun 15, 2021

Refactoring DiemID DIP into three DIPS: Diem ID (DIP-10), On-chain domain resolution (#187), and Reference ID exchange (#186).

@sunmilee sunmilee requested a review from davidiw June 15, 2021 10:04
Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

Should we call it sender's VASP or sender VASP?

dips/dip-10.md Outdated

The format of the command is:

```
{
"_ObjectType": "CommandRequestObject",
"command_type": "ReferenceIDCommand",
"command_type": "SenderInitiatedReferenceID",
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to rename this command, I suggest new name "ExchangePaymentAddress".
The reason is, for this command, what really need is for sender to get receiver's account address, when sender got receiver's DiemID.
We originally thought reference id is the data need to be exchanged. But I think reference id is an id for referencing the payment that uses DiemID and related account addresses.
It is better to not assume this is a command initiate the reference id, because if we had another command that needs exchange information between VASPs, and the command happens before this command, then we may have trouble.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, naming could definitely be improved!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can call it SenderInitiatedDiemIdExchange if we want to be specific to be diemID and not mention reference ID, since we're worried that reference ID exchange could happen in other commands

Copy link
Contributor

@xli xli Jun 22, 2021

Choose a reason for hiding this comment

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

I think SenderInitiatedDiemIDExchange is a good name, clearly described the business needs.

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

good for me

@davidiw davidiw merged commit 58f5bea into diem:main Jun 23, 2021
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.

3 participants