Claim allocations processes batches by sector #1337
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ZenGround0
approved these changes
Jul 30, 2023
| deal_space: deal_activation.nonverified_deal_space.clone(), | ||
| } | ||
| .zip(claim_res.sector_claims) | ||
| .map(|(sector_deals, sector_claim)| ext::market::DealSpaces { |
There was a problem hiding this comment.
Nice this is so much better
actors/verifreg/src/types.rs
Outdated
| pub all_or_nothing: bool, | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq, Default, Serialize_tuple, Deserialize_tuple)] | ||
| #[serde(transparent)] | ||
| pub struct SectorAllocationClaimResult { | ||
| pub struct SectorAllocationClaim { |
There was a problem hiding this comment.
nit: this name is a bit confusing together with SectorAllocationClaims
Maybe SectorClaimSummary?
|
I am seeing an integration test failure on |
anorth
commented
Jul 30, 2023
actors/verifreg/src/lib.rs
Outdated
| "all or nothing call contained failures: {}", | ||
| batch_info.to_string() | ||
| return Err(ActorError::checked( | ||
| batch_info.fail_codes[0].code, |
There was a problem hiding this comment.
This change to propagate the exit code from the firs failing item when all_or_nothing is true, rather than illegal argument, caused the integration test failure. I will revert it, though think perhaps propagating the code is better.
anorth
force-pushed
the
anorth/claim-allocation-sectors
branch
from
171eee4 to
662c463
Compare
anorth
force-pushed
the
anorth/claim-allocation-sectors
branch
from
811f8b6 to
dde209f
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1337 +/- ##
=======================================
Coverage 90.84% 90.85%
=======================================
Files 138 138
Lines 27350 27342 -8
=======================================
- Hits 24847 24841 -6
+ Misses 2503 2501 -2
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR reworks the verified registry ClaimAllocations method to follow the same pattern as the built-in market's ActivateDeals. Claims are grouped by sector, and a failed claim will cause all claims for that sector to fail. The all-or-nothing flag requests any failure to cause an abort.
This doesn't change the observable behaviour today because all-or-nothing is always set to true. But it does follow a pattern for batching now established (#1310) in the market actor, and prepares for the flag to be false when miner APIs support it (e.g. with direct data onboarding).