Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include median in master #2068

Merged
merged 8 commits into from
Sep 14, 2021
Merged

Include median in master #2068

merged 8 commits into from
Sep 14, 2021

Conversation

lrubiorod
Copy link
Contributor

No description provided.

@lrubiorod lrubiorod changed the title Include median Include median in master Sep 14, 2021
@tmpolaczyk
Copy link
Contributor

This was already reviewed in #2049

assert!(!config_oppose_0016.tapi.oppose_wip0014);
assert!(config_oppose_0016.tapi.oppose_wip0016);
assert!(!empty_config.tapi.oppose_wip0017);
assert!(config_oppose_0017.tapi.oppose_wip0017);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was also used to check that unset fields default to false. Can you add something like that again?

    assert!(!empty_config.tapi.oppose_wip0018);
    assert!(!config_oppose_0017.tapi.oppose_wip0018);

@lrubiorod
Copy link
Contributor Author

This was already reviewed in #2049

So... approved!! :P

assert_eq!(epoch, init_epoch_wip0014);
// The first block whose vote must be counted is the one from WIP0017-0018-0019
let init_epoch_wip0017_18_19 = 656640;
assert_eq!(epoch, init_epoch_wip0017_18_19);
// The TapiEngine was just created, there list of old_wips must be empty
assert_eq!(old_wips, HashSet::new());
// The list of active WIPs only contains the first, second, and third hard forks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// The list of active WIPs only contains the first, second, and third hard forks
// The list of active WIPs should match those defined in `wip_info`

Comment on lines 221 to 227
let active_wips = if let Some(active_wips) = active_wips {
active_wips.clone()
} else {
current_active_wips()
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this clone can be avoided, by doing something like this:

Suggested change
let active_wips = if let Some(active_wips) = active_wips {
active_wips.clone()
} else {
current_active_wips()
};
let current_wips;
let active_wips = if let Some(active_wips) = active_wips {
active_wips
} else {
current_wips = current_active_wips();
&current_wips
};

@lrubiorod lrubiorod merged commit 18b3410 into witnet:master Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants