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

Adds CommD as option in return value from ActivateDeals #1361

Merged
merged 2 commits into from
Aug 14, 2023

Conversation

anorth
Copy link
Member

@anorth anorth commented Aug 10, 2023

This avoids ProveReplicaUpdates calling the market twice consecutively.

Note this is based on master, it's not only part of direct data onboarding.

Closes #1308

@anorth anorth requested a review from ZenGround0 August 10, 2023 02:39
@anorth anorth force-pushed the anorth/pru-commd-refactor branch 2 times, most recently from 395fb85 to f7cbbd0 Compare August 10, 2023 04:38
@anorth
Copy link
Member Author

anorth commented Aug 10, 2023

@ZenGround0 I think this is complete but I haven't evaluated testing yet. I'll confirm the existing integration tests cover this and would fail if the CommD was computed incorrectly, and perhaps add a quick market unit test.

@anorth anorth force-pushed the anorth/pru-commd-refactor branch from f7cbbd0 to b67d97e Compare August 11, 2023 03:38
@anorth
Copy link
Member Author

anorth commented Aug 11, 2023

Tests upgraded to check CommD.

@anorth anorth marked this pull request as ready for review August 11, 2023 03:39
@codecov-commenter
Copy link

Codecov Report

Merging #1361 (b67d97e) into master (f46d4dc) will decrease coverage by 0.06%.
The diff coverage is 88.40%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1361      +/-   ##
==========================================
- Coverage   90.74%   90.68%   -0.06%     
==========================================
  Files         145      145              
  Lines       27354    27361       +7     
==========================================
- Hits        24822    24813       -9     
- Misses       2532     2548      +16     
Files Changed Coverage Δ
actors/market/src/types.rs 93.10% <ø> (-0.23%) ⬇️
actors/miner/src/ext.rs 68.96% <ø> (+1.22%) ⬆️
actors/miner/src/lib.rs 82.82% <78.37%> (-0.15%) ⬇️
actors/market/src/lib.rs 92.29% <100.00%> (+0.02%) ⬆️
integration_tests/src/expects.rs 95.76% <100.00%> (+0.03%) ⬆️
integration_tests/src/util/workflows.rs 99.32% <100.00%> (+<0.01%) ⬆️
test_vm/src/fakes.rs 77.35% <100.00%> (+3.44%) ⬆️

... and 4 files with indirect coverage changes

Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

Code change looks good, will attempt to look through tests in the next 24 hours.


use fil_actor_miner::{
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do these rearrangments come from? Is this some change in cargo fmt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for picking this up. It comes from my IDE's "Optimize Imports" function, which regroups imports into std, external crates, workspace crates, etc. Rustfmt by default doesn't care about import grouping, only alphabetical order within a group.

The group imports feature could be turned on, which would I think then completely specify the grouping and order (similar but slightly different to my IDE). After a one-time churn, this could reduce noise here in the future.

In the meantime I will attempt to avoid regrouping imports this way.

@anorth anorth added this pull request to the merge queue Aug 14, 2023
Merged via the queue into master with commit c5cb94c Aug 14, 2023
@anorth anorth deleted the anorth/pru-commd-refactor branch August 14, 2023 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Avoid redundant VerifyDealsForActivation call in ProveReplicaUpdates
3 participants