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

Enable Forward Compatibility methods and add tests #650

Merged
merged 6 commits into from
Sep 21, 2022

Conversation

Kubuxu
Copy link
Contributor

@Kubuxu Kubuxu commented Sep 14, 2022

Resolves: #521
Resolves: #13

@Kubuxu Kubuxu force-pushed the feat/forward-compat-tests branch from c83a0a3 to 4da1b1f Compare September 14, 2022 15:29
@Kubuxu Kubuxu requested review from ZenGround0 and anorth September 14, 2022 15:29
@codecov-commenter
Copy link

Codecov Report

Merging #650 (c83a0a3) into master (fe531cb) will increase coverage by 0.21%.
The diff coverage is n/a.

❗ Current head c83a0a3 differs from pull request most recent head 4da1b1f. Consider uploading reports for the commit 4da1b1f to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #650      +/-   ##
==========================================
+ Coverage   82.21%   82.42%   +0.21%     
==========================================
  Files          88       88              
  Lines       18306    18308       +2     
==========================================
+ Hits        15050    15091      +41     
+ Misses       3256     3217      -39     
Impacted Files Coverage Δ
actors/miner/src/lib.rs 71.77% <ø> (+0.83%) ⬆️
state/src/check.rs 87.23% <0.00%> (-0.43%) ⬇️
actors/power/src/lib.rs 84.11% <0.00%> (-0.43%) ⬇️
actors/market/src/lib.rs 81.64% <0.00%> (+0.09%) ⬆️
actors/miner/src/types.rs 91.11% <0.00%> (+2.22%) ⬆️
actors/cron/src/lib.rs 93.10% <0.00%> (+3.44%) ⬆️
actors/miner/src/commd.rs 89.65% <0.00%> (+20.68%) ⬆️

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

This looks good so far, except for weirdness in the harness methods which I expect you can simplify.

In a follow-up, could you please also add an integration test under test_vm/tests

@Kubuxu Kubuxu requested a review from anorth September 19, 2022 12:54
@Kubuxu Kubuxu force-pushed the feat/forward-compat-tests branch from 661a748 to fa17fb6 Compare September 19, 2022 14:13
Jakub Sztandera added 4 commits September 19, 2022 16:14
Signed-off-by: Jakub Sztandera <[email protected]>
Signed-off-by: Jakub Sztandera <[email protected]>
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu Kubuxu force-pushed the feat/forward-compat-tests branch from fa17fb6 to d4b6998 Compare September 19, 2022 14:26
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

LGTM. I would like @ZenGround0 to take a look if possible, but if he doesn't get to it in a couple of days, let's merge and he can review afterwards.

@@ -546,7 +550,7 @@ fn upgrade_bad_post_dispute() {
let store = &MemoryBlockstore::new();
let policy = Policy::default();
let (v, sector_info, worker, miner_id, deadline_index, partition_index, _) =
create_miner_and_upgrade_sector(store);
create_miner_and_upgrade_sector(store, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not test v2 for these tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it is unrelated to the changes, I can add these tests if we want to.

@Kubuxu Kubuxu enabled auto-merge (squash) September 21, 2022 14:12
@Kubuxu Kubuxu merged commit fa94365 into master Sep 21, 2022
@Kubuxu Kubuxu deleted the feat/forward-compat-tests branch September 21, 2022 14:26
Kubuxu pushed a commit that referenced this pull request Sep 22, 2022
Signed-off-by: Jakub Sztandera <[email protected]>
Kubuxu pushed a commit that referenced this pull request Sep 22, 2022
Signed-off-by: Jakub Sztandera <[email protected]>

Signed-off-by: Jakub Sztandera <[email protected]>
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Jan 31, 2023
)

* Enable Forward Compatibility methods and add tests

Signed-off-by: Jakub Sztandera <[email protected]>

* Address review

Signed-off-by: Jakub Sztandera <[email protected]>

* Add tests for ReplicaUpdate2

Signed-off-by: Jakub Sztandera <[email protected]>

* Remove duplicate pub use

Signed-off-by: Jakub Sztandera <[email protected]>

* Test v2 pre-commit in integration test

Signed-off-by: Jakub Sztandera <[email protected]>

Signed-off-by: Jakub Sztandera <[email protected]>
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Jan 31, 2023
Signed-off-by: Jakub Sztandera <[email protected]>

Signed-off-by: Jakub Sztandera <[email protected]>
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.

Implement FIP-0041 forward compatibility shim for pre-commit Remove old CC upgrade/replace params
4 participants