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.

Sign up for GitHub

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

Jump to bottom

feat: Return cell data in get_live_cell rpc #1527

Merged
merged 1 commit into from
Sep 4, 2019
Merged

feat: Return cell data in get_live_cell rpc #1527

merged 1 commit into from
Sep 4, 2019

Conversation

TheWaWaR
Copy link
Contributor

@TheWaWaR TheWaWaR commented Sep 3, 2019 •
edited
Loading

BREAKING CHANGE: get_live_cell add with_data argument and result structure changed.

@TheWaWaR TheWaWaR requested a review from a team September 3, 2019 09:35
@nervos-bot nervos-bot bot added the breaking change The feature breaks consensus, database, message schema or RPC interface. label Sep 3, 2019
@nervos-bot
Copy link

nervos-bot bot commented Sep 3, 2019

@zhangsoledad is assigned as the chief reviewer

quake
quake requested changes Sep 3, 2019
View reviewed changes
rpc/src/module/chain.rs
@@ -44,7 +44,7 @@ pub trait ChainRpc {
) -> Result<Vec<CellOutputWithOutPoint>>;

#[rpc(name = "get_live_cell")]
fn get_live_cell(&self, _out_point: OutPoint) -> Result<CellWithStatus>;
fn get_live_cell(&self, _out_point: OutPoint, _with_data: bool) -> Result<CellWithStatus>;
Copy link
Member

Choose a reason for hiding this comment

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

change with_data to optional param:

Suggested change
fn get_live_cell(&self, _out_point: OutPoint, _with_data: bool) -> Result<CellWithStatus>;
fn get_live_cell(&self, _out_point: OutPoint, _with_data: Option<bool>) -> Result<CellWithStatus>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better explicit here, if we add another more argument in the future, it will make user confused.

@quake
Copy link
Member

quake commented Sep 3, 2019

BREAKING CHANGE: get_live_cell add with_data argument and result structure changed.

we can use the optional param, it will keep rpc compatibility.

quake
quake reviewed Sep 3, 2019
View reviewed changes
rpc/json/rpc.json Outdated Show resolved Hide resolved
@doitian doitian added this to 👀 Awaiting review in CKB - Pull Requests Sep 3, 2019
@TheWaWaR
Copy link
Contributor Author

TheWaWaR commented Sep 3, 2019

BREAKING CHANGE: get_live_cell add with_data argument and result structure changed.

we can use the optional param, it will keep rpc compatibility.

get_live_cell is not compatible with old version any more, because the return data structure has changed.

u2
u2 suggested changes Sep 4, 2019
View reviewed changes
Copy link
Contributor

@u2 u2 left a comment

Choose a reason for hiding this comment

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

According to the code implementation, if we do not set the second argument with_data, it will return the wrong argument number, it's inconsistent with the RPC document.

@TheWaWaR
Copy link
Contributor Author

TheWaWaR commented Sep 4, 2019

According to the code implementation, if we do not set the second argument with_data, it will return the wrong argument number, it's inconsistent with the RPC document.

Oops, you are right.

@TheWaWaR
feat: Return cell data in get_live_cell rpc
bb628be 
BREAKING CHANGE: get_live_cell add with_data argument and result structure changed
@keroro520 keroro520 requested a review from u2 September 4, 2019 04:26
CKB - Pull Requests automation moved this from 👀 Awaiting review to ✅ Reviewer approved Sep 4, 2019
@u2
Copy link
Contributor

u2 commented Sep 4, 2019

bors r=quake,u2,keroro520

@nervos-bot nervos-bot bot added the s:ready-to-merge Status: Waiting to be merged. label Sep 4, 2019
bors bot added a commit that referenced this pull request Sep 4, 2019
@bors @TheWaWaR
Merge #1527
a0dae62 
1527: feat: Return cell data in get_live_cell rpc r=quake,u2,keroro520 a=TheWaWaR

BREAKING CHANGE: `get_live_cell` add `with_data` argument and result structure changed.

Co-authored-by: Linfeng Qian <thewawar@gmail.com>
@bors
Copy link
Contributor

bors bot commented Sep 4, 2019

Build succeeded

  • continuous-integration/travis-ci/push

@bors bors bot merged commit bb628be into nervosnetwork:develop Sep 4, 2019
CKB - Pull Requests automation moved this from ✅ Reviewer approved to Done Sep 4, 2019
@TheWaWaR TheWaWaR deleted the add-get-live-cell-data-rpc branch June 10, 2022 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quake quake quake approved these changes

@u2 u2 u2 approved these changes

@keroro520 keroro520 keroro520 approved these changes

Assignees

@zhangsoledad zhangsoledad

Labels
breaking change The feature breaks consensus, database, message schema or RPC interface. s:ready-to-merge Status: Waiting to be merged.
Projects
CKB - Pull Requests
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@TheWaWaR @quake @u2 @keroro520 @zhangsoledad