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

DIP 161 Optional Result Field #165

Merged
merged 4 commits into from
Apr 19, 2021
Merged

DIP 161 Optional Result Field #165

merged 4 commits into from
Apr 19, 2021

Conversation

sunmilee
Copy link
Contributor

@sunmilee sunmilee commented Apr 8, 2021

Adding DIP 161: optional result field in off-chain protocol as defined in this issue #161

@sunmilee sunmilee requested review from davidiw and xli April 8, 2021 23:21
dips/dip-161.md Outdated Show resolved Hide resolved
dips/dip-161.md Outdated Show resolved Hide resolved
Copy link
Contributor

@CapCap CapCap left a comment

Choose a reason for hiding this comment

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

this is awesome!

davidiw
davidiw previously requested changes Apr 9, 2021
dips/dip-161.md Show resolved Hide resolved
dips/dip-161.md Show resolved Hide resolved
dips/dip-161.md Outdated Show resolved Hide resolved
@davidiw
Copy link
Contributor

davidiw commented Apr 9, 2021

I don't think there's any big blockers here, but hit me up or dismiss review so we can get it in

@sunmilee sunmilee requested a review from davidiw April 13, 2021 03:56
Copy link
Contributor

@dahliamalkhi dahliamalkhi left a comment

Choose a reason for hiding this comment

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

LGTM!

@sunmilee sunmilee dismissed davidiw’s stale review April 13, 2021 20:01

Addressed concern

dips/dip-161.md Outdated Show resolved Hide resolved
dips/dip-161.md Outdated Show resolved Hide resolved
dips/dip-161.md Outdated Show resolved Hide resolved
dips/dip-161.md Outdated Show resolved Hide resolved
dips/dip-161.md Outdated Show resolved Hide resolved
dips/dip-161.md Outdated
Comment on lines 43 to 51

**CommandResultObject:**

| Field | Type | Required? | Description |
|------- |------ |----------- |------------- |
| _ObjectType | str | Y | Type of command result object that uniquely defines the shape or other fields within the result |
| recipient_address | str | N | Receiver's on-chain address |

Other result types may be added if there are other use cases for the result field.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would argue we don't need to define this and should leave delete this

Suggested change
**CommandResultObject:**
| Field | Type | Required? | Description |
|------- |------ |----------- |------------- |
| _ObjectType | str | Y | Type of command result object that uniquely defines the shape or other fields within the result |
| recipient_address | str | N | Receiver's on-chain address |
Other result types may be added if there are other use cases for the result field.

@sunmilee sunmilee requested review from davidiw and xli April 16, 2021 19:39
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.

5 participants