Uh oh!
There was an error while loading. Please reload this page.
Make Polkadot use the Substrate traity libraries - #105
Conversation
aa041e1 to
6b7b944Compare| pub fn signature(&self) -> &Signature { | ||
| &self.0.signature | ||
| } | ||
| mod tests { |
There was a problem hiding this comment.
meh - they're just serialisation tests; not sure they really belong here (i.e. rather than in generic) but didn't want to throw away the code.
There was a problem hiding this comment.
some of the tests are for things like transactions coming after timestamp, etc. I would prefer to remove anything rather than comment it out because it's in the git history anyway.
| } | ||
| } | ||
| pub type Header = generic::Header<BlockNumber, Hash, Vec<u8>>; |
There was a problem hiding this comment.
seems like changes from the end of the last PR were lost (docs and presumably all the other small cleanups)
There was a problem hiding this comment.
may have got clobbered. my god i hate that git can't support merging squashed commits.
rphmeier
commented
Apr 4, 2018
All the other changes in the recent cleanup commits were lost as well -- maybe they can be replayed on top. |
gavofyork
commented
Apr 4, 2018
those were not lost - they were never there. i had previously split out the runtime. |
gavofyork
commented
Apr 4, 2018
(i already replayed them - they're already in there) |
| #[derive(Eq, PartialEq, Clone)] | ||
| #[derive(Eq, PartialEq, Clone, Default)] | ||
| #[cfg_attr(feature = "std", derive(Debug, Serialize))] | ||
| pub struct Ed25519Signature(H512); |
There was a problem hiding this comment.
this isn't a trait, and makes the type alias Signature which references this confusing
There was a problem hiding this comment.
it's not meant to be a trait. not sure which usage of Signature you mean, but if it's in a more general context than ed25519 or Polkadot than it should be removed in favour of this. (i would note that it's perfectly reasonable for polkadot::Signature to mean Ed25519Signature.)
There was a problem hiding this comment.
I mean that it's in the traits module and type polkadot::Signature = traits::Ed25519Signature is misleading.
| fn is_empty(&self) -> bool; | ||
| } | ||
| impl<T: Default + PartialEq> MaybeEmpty for T { |
There was a problem hiding this comment.
this blanket impl is overbroad. The Default trait doesn't imply emptiness at all. Are there some types in particular we want this for?
There was a problem hiding this comment.
yes - specifically for PublicAux, for which AccountId is used in production environments (but tests use u64 hence this generalisation). it should eventually be refactored (as i note somewhere near) so that it's actually only implemented for Option<AccountId> and u64, but for now, it gets the job done without requiring substantial changes in how PublicAux is dispatched
| if unchecked.extrinsics[0].is_signed() { | ||
| return Err(unchecked); | ||
| } | ||
| if let Call::Timestamp(TimestampCall::set(_)) = unchecked.extrinsics[0].extrinsic.function {} else { |
There was a problem hiding this comment.
could rewrite this as
ifletCall::Timestamp(...) = ...{Ok(...)}else{Err(...)}There was a problem hiding this comment.
would work right now, but not in general since we'd expect more validity tests to be added here.
There was a problem hiding this comment.
that's fine. the if let X {} else { ... } is unidiomatic so I'd prefer a match statement.
| /// Create a new block, skipping any high-level well-formedness checks. WARNING: This could | ||
| /// result in internal functions panicking if the block is, in fact, not well-formed. | ||
| pub fn force_from(known_good: Block) -> Self { |
There was a problem hiding this comment.
the main problem with this is that the errors occur in such a way that it's difficult to track down the point at which the programmer error was actually made.
We're changing types like HeaderView which are supposed to take only trusted data in Parity-Ethereum to solve a similar problem by having the function take the file name and line number and having the view carry those around to incorporate in panic messages, so we get the cause instead of just the symptom. We also provide a macro which invokes this constructor using the file!() and line!() macros internally.
I would be in support of a similar change being done here (it only adds a pointer and u32 to the size of the structure)
| if let Call::Timestamp(TimestampCall::set(t)) = self.0.extrinsics[0].extrinsic.function { | ||
| t | ||
| } else { | ||
| unreachable!(); |
| let mut roles_gua = roles_val.clone(); | ||
| let h = <system::Module<T>>::random_seed(); | ||
| let mut seed = Vec::<u8>::new().and(&h).and(b"validator_role_pairs").blake2_256(); |
There was a problem hiding this comment.
h.to_vec().and(...) seems cleaner
* Update to substrate alpha.7 * Remove gas limit from contracts put code * Rename SystemEvent::ReapedAccount to KilledAccount * Log debug event received before attempting to decode * Temporary registration of Balance type before paritytech#102 merged * Show contract test errors, increase instantiate gas_limit
No description provided.