Conversation
|
@andresilva still has the commit in #2801 . |
Demi-Marie
left a comment
Demi-Marie
left a comment
There was a problem hiding this comment.
The commit message shoule explain what code changes were needed and why.
| state: RoundState<Block::Hash, NumberFor<Block>>, | ||
| base: (Block::Hash, NumberFor<Block>), | ||
| votes: Vec<SignedMessage<Block>>, | ||
| votes: &HistoricalVotes<Block::Hash, NumberFor<Block>, Self::Signature, Self::Id>, |
There was a problem hiding this comment.
What is the reason for this change?
There was a problem hiding this comment.
It is an implementation of the Environment trait from finality-grandpa. The trait definition is changed, so the implementation needs to be updated.
There was a problem hiding this comment.
|
@DarkEld3r I merged master into #2801 when master still had this change in. It's fine to merge it in this PR instead so you can keep attribution (I'll rebase and remove it from my PR once it is in). The issue is around synchronization of merges: we can't release |
|
also parity-codec 4.1 needs the fix of for crafted input that trigger huge allocation paritytech/parity-scale-codec#109 |
|
@DarkEld3r is this good to be merged now? |
|
@gavofyork I suppose it makes sense to wait a little bit more for |
Updated. |
core/consensus/slots/Cargo.toml
Outdated
|
|
||
| [dependencies] | ||
| codec = { package = "parity-codec", version = "3.2" } | ||
| codec = { package = "parity-codec", version = "4.1" } |
There was a problem hiding this comment.
Maybe not now but:
don't we want to put some guidelines on import names? sometimes it can be confusing. I bet you can find the crate core/sr-primitives along the code base with names:
primitivesr-primitivessubstrate-primitives- ...
There was a problem hiding this comment.
I like the idea, though it will be hard to apply strict rules everywhere because we have a lot of similar names (for example, "primitives").
andresilva
left a comment
andresilva
left a comment
There was a problem hiding this comment.
LGTM. We will have an unreleased dependency in master (finality-grandpa), but this will be dealt with shortly after #2801 is merged.
|
Can we not change the destination branch to @andresilva's grandpa branch for #2801? Merge it into this branch and merge into master with #2801 together. |
|
@gavofyork I suppose it is OK to merge now. |
* Update codec version to the 4.1 version * Bump impl_version * Update lock files * Update codec to 4.1.1 version * Bump impl version
8 participants
#2855 was accidentally merged (and reverted) before it was ready, so here is an updated version.