signed wrapper #1283
signed wrapper #1283
Conversation
This is strictly an addition as of this commit; nothing is yet changed in existing behavior.
github-actions
bot
added
the
A3-in_progress
There was a problem hiding this comment.
@rphmeier if you have time, I'd appreciate an early look at this to ensure that this is the right direction to take this.
Next up: replace SignedStatement, SignedAvailabilityBitfield with the typedefs from the guide and ensure everything still builds.
primitives/src/parachain.rs
Outdated
| // because there's no blanket impl of `AsRef<T> for T`. In the end, we just invent our | ||
| // own trait which does what we need: EncodeAs. | ||
| impl<Payload: EncodeAs<RealPayload>, RealPayload: Encode> Signed<Payload, RealPayload> { | ||
| fn payload_data(payload: &Payload, context: SigningContext) -> Vec<u8> { |
There was a problem hiding this comment.
I'm not really a fan of inherent impls that aren't constructors TBH
There was a problem hiding this comment.
Not sure if this is considered bad practice, but my intuition is that it generally is, although I don't know what resource to reference on that
There was a problem hiding this comment.
The reason this is an inherent impl is that it's used within the constructor. Users shouldn't care; it's private.
| // the payload, and we don't want that. We can't bound it on `Payload: AsRef<RealPayload>` | ||
| // because there's no blanket impl of `AsRef<T> for T`. In the end, we just invent our | ||
| // own trait which does what we need: EncodeAs. | ||
| impl<Payload: EncodeAs<RealPayload>, RealPayload: Encode> Signed<Payload, RealPayload> { |
There was a problem hiding this comment.
It seems like the EncodeAs machinery could be replaced with a simple From bound, but actually not, because you'd have to do some cloning internally for types like AvailabilityBitfield.
Playground to avoid this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f734b13e93d6a7baa8b19dab78df345f
But it gets a bit messy
There was a problem hiding this comment.
You have to use the trait because you would need HKT to do this with pure closures.
There was a problem hiding this comment.
Exactly right on the reason we're using EncodeAs instead of From/Into.
I think we may have been overthinking the solution, though; my new attempt is in 234eca5.
This isn't an ideal solution, because it depends on the implementation details of how SCALE encodes tuples, but OTOH that behavior seems unlikely to change anytime soon.
coriolinus
added
B0-silent
coriolinus
added
A0-please_review
Not sure why cargo check didn't catch this earlier; oh well.
rphmeier
added
A8-mergeoncegreen
and removed
A0-please_review
node/primitives/src/lib.rs
Outdated
| } | ||
| } | ||
| } | ||
| pub type SignedStatement = Signed<Statement, CompactStatement>; |
There was a problem hiding this comment.
We need to be careful that runtime APIs accept Signed<CompactStatement>, not Signed<Statement>.
I propose calling this SignedFullStatement
|
@coriolinus I submitted 75ce529 to fix tests and create a clear type boundary between signed statements and their "full" counterparts. I also merged with master in an effort to get CI to pass. Please merge if my changes are acceptable |
Co-authored-by: Robert Habermeier <[email protected]>
|
Happy to merge once green! Really not sure what's going on with CI; I was wrestling with that all afternoon yesterday, with no success. 75ce529 seems fine to me, but it looks like we only ever use One nice property: we could |
Yeah, we don't do that now, but I'd imagine we will use it in the dispute-resolution process. The |
Closes #1282.
Extracts signing behavior from concrete types into a wrapper where the behavior can be written exactly once.