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

Add reference ID exchange object types #274

Merged
merged 3 commits into from
Apr 27, 2021
Merged

Add reference ID exchange object types #274

merged 3 commits into from
Apr 27, 2021

Conversation

sunmilee
Copy link
Contributor

@sunmilee sunmilee commented Apr 23, 2021

Adding data types for reference ID exchange as defined by DIP10

@sunmilee sunmilee requested a review from xli April 23, 2021 07:28
@sunmilee sunmilee force-pushed the ref_id branch 2 times, most recently from beef99c to cff14cd Compare April 23, 2021 08:24
@@ -134,3 +160,18 @@ class CommandResponseObject:
error: typing.Optional[OffChainErrorObject] = datafield(default=None)
# The Command identifier to which this is a response.
cid: typing.Optional[str] = datafield(default=None)
# An result JSON object that may be defined when status == "success"
result: typing.Optional[OffChainResultObject] = datafield(default=None)
Copy link
Contributor

Choose a reason for hiding this comment

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

use dict # pyre-ignore for type definition.
when decoding the response object, result is unknown, as caller should base on request command type to decide what is response type, thus it is unknown when we decode CommandResponseObject.
For example: PaymentCommand response should not have result = ReferenceIDCommandResponse.
It is more flexible to let caller to decode the dict to specific type.

@@ -124,6 +137,19 @@ class OffChainErrorObject:
message: typing.Optional[str] = datafield(default=None)


@dataclass(frozen=True)
class OffChainResultObject:
Copy link
Contributor

Choose a reason for hiding this comment

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

change this to ReferenceIDCommandResultObject, may also update https://github.com/diem/dip/pull/164/files "CommandResultObject" to use same name,

When there are multiple result objects, it is better we don't mix their fields.

}
)
# ReferenceIDCommandResponse: Receiver's onchain account identifier
receiver_address: typing.Optional[str] = datafield(default=None)
Copy link
Contributor

Choose a reason for hiding this comment

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

this field is required, not optional

@@ -134,3 +160,18 @@ class CommandResponseObject:
error: typing.Optional[OffChainErrorObject] = datafield(default=None)
# The Command identifier to which this is a response.
cid: typing.Optional[str] = datafield(default=None)
# An result JSON object that may be defined when status == "success"
result: dict # pyre-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

typing.Optional[dict]

@sunmilee sunmilee force-pushed the ref_id branch 2 times, most recently from c307257 to 1c61c6d Compare April 26, 2021 19:21
@xli
Copy link
Contributor

xli commented Apr 26, 2021

may add a test to test_offchain_types for decoding CommandResponseObject with result dict

@sunmilee sunmilee requested a review from xli April 27, 2021 07:16
@sunmilee
Copy link
Contributor Author

@xli want to take a look at the test?

@xli xli merged commit 5bbe291 into diem:master Apr 27, 2021
@sunmilee sunmilee mentioned this pull request May 11, 2021
24 tasks
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

2 participants