feat: substrate upgrade. monthly-2022-06 to monthly-2022-12 #2682
Conversation
kylezs
force-pushed
the
kyle/substrate-upgrade
branch
from
16d8c6d
to
110a17f
Compare
engine/src/dot/witnesser.rs
Outdated
| (block_hash, block_number, events.iter().filter_map(|event_details| { | ||
| match event_details { | ||
| Ok(event_details) => { | ||
| if event_details.pallet_name() == ProxyAdded::PALLET && event_details.variant_name() == ProxyAdded::EVENT { |
There was a problem hiding this comment.
Looks like event_details.phase() is used as the first value in the tuple regardless of the event type. I think the code would be more clear if we first constructed the appropriate EventWrapper type and then simply mapped Some(event) to Some((event_details.phase(), event). Also, could we not use a match expression here (for comparing pallet name and variant name)?
There was a problem hiding this comment.
Made the map change to dedup the calls to event_details.phase(). Don't think making it a match particular improves anything, so left that
There was a problem hiding this comment.
Usually we prefer match over a series of else if. Also I thought it would look cleaner without == in both places, and without having to repeat event_details.pallet_name() and event_details.variant_name() in every line. With else if it is not immediately clear that we are matching on the variant name and the pallet name in every branch, every else if branch could have an arbitrary conditional expression. match makes this explicit.
This is what I have in mind:
match (event_details.pallet_name(), event_details.variant_name()) {
(ProxyAdded::PALLET, ProxyAdded::EVENT) => ...
}But maybe I'm missing something and this does not work? In any case, happy for you to leave it as is.
There was a problem hiding this comment.
I agree with this, will make the change in Kyle's absence.
There was a problem hiding this comment.
Not sure what I was thinking by the first mention of using a match statement, but yeah I do agree that way you've laid out is better 👍
| @@ -455,8 +455,7 @@ mod tests { | |||
| )); | |||
|
|
|||
| // Now it is okay to distribute the shares | |||
|
|
|||
| let _agg_pubkey: Point = coeff_commitments.iter().map(|(_idx, c)| c.commitments.0[0]).sum(); | |||
| let _agg_pubkey: Point = coeff_commitments.values().map(|c| c.commitments.0[0]).sum(); | |||
There was a problem hiding this comment.
Not sure if this was intentional, but I do kind of like having this like just to make it clear what's happening in the test (e.g. that we agree on the public key using commitments). No strong opinion though.
| @@ -89,44 +89,3 @@ fn check_threshold_calculation() { | |||
| assert_eq!(failure_threshold_from_share_count(3), 2); | |||
| assert_eq!(failure_threshold_from_share_count(4), 2); | |||
| } | |||
|
|
|||
| pub fn clean_hex_address<const LEN: usize>(address_str: &str) -> Result<[u8; LEN], &'static str> { | |||
There was a problem hiding this comment.
just moved into the with_std section, since hex::decode requires alloc (in std)
| @@ -44,10 +44,11 @@ where | |||
| let backup_stake = amount / QUANTISATION_FACTOR; | |||
| let reward = min( | |||
| average_authority_reward, | |||
| multiply_by_rational( | |||
| multiply_by_rational_with_rounding( | |||
There was a problem hiding this comment.
I think we should round down.
| optional = true | ||
| tag = 'chainflip-monthly-2022-06+01' | ||
|
|
||
| [dev-dependencies.sp-core] |
There was a problem hiding this comment.
There are a few places where dev dependencies were accidentally converted into regular dependencies. I think maybe my fault. I'll go through an correct this.
Excess whitspace Some default-features = false Redundant # Dev dependencies comments Put deps back in dev-dependencies Order deps more consistently
There were quite a lot of changes here. I'll review this myself too. Will mark some areas that I wasn't sure of, or where, as a result of the upgrade, shifts in how the logic works were required, so extra attention can be paid there.
I've assigned @dandanlen for SC stuff and @msgmaxim for CFE side to decrease amount on each person 😅 . Also may tag others in sections relevant to them for those sections.
Tomorrow I'll run on a testnet too once I've got the final little things sorted and it's building on CI.