-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Beefy: use VersionedFinalityProof instead of SignedCommitment #11962
Beefy: use VersionedFinalityProof instead of SignedCommitment #11962
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, thank you for contributing!
There's a few nits, and an open question on whether we should also change public facing RPC API (breaking change). I think we have to do it eventually if we really want to support versioned proofs, and better to do it now before deploying to production networks.
client/beefy/rpc/src/lib.rs
Outdated
let stream = self.signed_commitment_stream.subscribe().map(|vfp| match vfp { | ||
VersionedFinalityProof::V1(sc) => | ||
notification::EncodedSignedCommitment::new::<Block>(sc), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually change the exposed RPCs too. User facing API should support versioning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change the exposed RPCs now.
client/beefy/src/import.rs
Outdated
// `VersionedFinalityProof`. | ||
VersionedFinalityProof::V1(signed_commitment) => self | ||
BeefyVersionedFinalityProof::<Block>::V1(signed_commitment) => self | ||
.justification_sender | ||
.notify(|| Ok::<_, ()>(signed_commitment)) | ||
.notify(|| Ok::<_, ()>(BeefyVersionedFinalityProof::<Block>::V1(signed_commitment))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to unpack here just to pack it back in V1
, just directly send the justification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I made confused here. 😄
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
2.Fix typos. 3.Change the exposed RPC API to support versioned proofs.
…f-SignedCommitment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…f-SignedCommitment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
Please add to the PR description the fact that BEEFY RPC API changed and should be included in release notes.
Ok . Done. |
@hzy1919 thanks for contributing! you can add a polkadot address for a tip |
bot merge |
Error: Github API says #11962 is not mergeable |
@acatangiu You're welcome . I have added my polkadot address in the PR description. |
/tip small |
@acatangiu You are not allowed to request a tip. Only shawntabrizi, gavofyork, rphmeier, athei, andresilva, arkpar, bkchr, eskimor, drahnr, dvdplm, robbepop, cmichi, tomaka, pepyakin, tomusdrw, kianenigma, jacogr are allowed. |
/tip small |
@bkchr A small tip was successfully submitted for hzy1919 (15wPGN5vysDW1HYWfZRjQQhv2ktpfLMMuiJNMRKcodwPfM14 on polkadot). https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips |
…tech#11962) * beefy: use VersionedFinalityProof instead of SignedCommitment. * Change the exposed RPC API to support versioned proofs. Co-authored-by: Adrian Catangiu <adrian@parity.io>
…tech#11962) * beefy: use VersionedFinalityProof instead of SignedCommitment. * Change the exposed RPC API to support versioned proofs. Co-authored-by: Adrian Catangiu <adrian@parity.io>
Fixes #11838
And change BEEFY RPC public API .
polkadot companion: paritytech/polkadot#5852
polkadot address: 15wPGN5vysDW1HYWfZRjQQhv2ktpfLMMuiJNMRKcodwPfM14