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

SubmitAndWatch: Return ExtrinsicReport instead of block hash #386

Merged
merged 26 commits into from
Dec 23, 2022

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Dec 20, 2022

❗ Breaks submit_and_watch_extrinsic and submit_extrinsic interfaces:

  • extrinsic submission now expects an encoded extrinsic. Hex encoding is no longer allowed / required. The hex encoding is done in the functions themselves. This is needed to calculate the extrinsic hash.
  • The return value of submit_and_watch_extrinsic has been updated to ExtrinsicReport which includes the extrinsic hash, the (maybe) block hash, the transaction status and, possibly, the associated events. This has the following advantages:
    • Clearly distinguishes between the xt hash (returned by submit_extrinsic) and the block hash previously returned.
    • Allows to return all associated items of the extrinsic to the caller.

Preparatory step for #288.

@haerdib haerdib self-assigned this Dec 20, 2022
@haerdib haerdib changed the title SubmitAndWatch: Return report instead of block hash SubmitAndWatch: Return ExtrinsicReport instead of block hash Dec 20, 2022
src/api/mod.rs Show resolved Hide resolved
@haerdib haerdib added E2-breaksapi F7-enhancement Enhances an already existing functionality labels Dec 20, 2022
@haerdib haerdib marked this pull request as ready for review December 20, 2022 11:52
Copy link
Contributor

@echevrier echevrier left a comment

Choose a reason for hiding this comment

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

LGTM

@haerdib haerdib requested review from clangenb and removed request for clangenb December 21, 2022 16:13
Copy link
Collaborator

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Looks very nice and clean, I like it. I have just some questions. :)

src/api/rpc_api/chain.rs Show resolved Hide resolved
src/api/rpc_api/mod.rs Outdated Show resolved Hide resolved
src/api/rpc_api/author.rs Show resolved Hide resolved
src/api/rpc_api/author.rs Outdated Show resolved Hide resolved
src/api/mod.rs Show resolved Hide resolved
src/api/mod.rs Show resolved Hide resolved
Copy link
Collaborator

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Very nice, I only have a comment left.

src/api/mod.rs Show resolved Hide resolved
src/api/rpc_api/author.rs Show resolved Hide resolved
DispatchError::decode_from(event_details.field_bytes(), self.metadata());
return Err(Error::Dispatch(dispatch_error))
}
event_details.check_failed()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am uncertain if I agree with the naming. I propose a few here:

  • check_if_failed
  • xt_successful
  • xt_failed

What do you think?

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 was thinking about the first one as well. Let me adapt.

Copy link
Collaborator

@clangenb clangenb left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me now!

@haerdib haerdib merged commit e5162ea into master Dec 23, 2022
@haerdib haerdib deleted the bh/return-report-instead-hash branch December 23, 2022 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E2-breaksapi F7-enhancement Enhances an already existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants