Skip to content

New Release#474

Merged
ziscky merged 4 commits into
mainfrom
dev
Aug 14, 2025
Merged

New Release#474
ziscky merged 4 commits into
mainfrom
dev

Conversation

@ziscky
Copy link
Copy Markdown
Contributor

@ziscky ziscky commented Aug 11, 2025

🔗 zboto Link

Summary by CodeRabbit

  • New Features

    • Added actor_address to deal proposals, activations and space records.
    • Switched from aggregated to per-deal records with per-deal IDs and GroupDealIDs to preserve sector grouping.
    • Event generator now accepts configuration to control deals event behavior.
  • Refactor

    • Optional robust address consolidation for provider/client addresses.
    • Renamed DealsMessages.Value → Data (API/JSON key changed).
    • Numeric fields (price, collateral, space/weight) now support larger/binary-safe values.

* fix: panic for slice access

* fix: pieceActivationManifest consolidation

* fix: add actor_address to all structs
@ziscky ziscky requested a review from emmanuelm41 August 11, 2025 14:20
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Aug 11, 2025

Walkthrough

Emit per-deal records for deal verification and activation, add ActorAddress to proposals/activations/space info, replace aggregated DealIDs with per-deal DealID + GroupDealIDs, rename DealsMessages.Value → Data, change several numeric fields to big.Int with gorm annotations, and thread parser.Config into the deals event generator.

Changes

Cohort / File(s) Summary
Types schema changes
types/deals.go
Renamed DealsMessages.ValueData. Added ActorAddress string to DealsProposals, DealsActivations, and DealsSpaceInfo. Replaced DealIDs []uint64 with DealID uint64 + GroupDealIDs []uint64 on DealsSpaceInfo. Several numeric fields (price/collateral/space) use *big.Int with gorm UInt256 metadata.
Verify & Activate parsing
tools/deals/activations.go
VerifyDealsForActivation and ActivateDeals now emit per-deal DealsActivations and per-deal DealsSpaceInfo entries (one entry per dealID). Per-deal records include DealID, GroupDealIDs, and ActorAddress; ID construction uses the individual dealID; NV3+/NV20 branching preserved with SpaceAsWeight for NV3+ flows.
Proposals & address consolidation
tools/deals/proposal.go
Populate DealsProposals.ActorAddress from transaction From; added robust address consolidation via consolidateAddress when configured; handle verified-deal extraction differently on mainnet for NV14; store price per epoch as big-int (no Uint64 conversion).
Messages payload rename
tools/deals/messages.go
Assign DealsMessages.Data = tx.TxMetadata (field renamed from ValueData).
Event generator & plumbing
tools/deals/deals.go, parser/v1/parser.go, parser/v2/parser.go
NewEventGenerator signature extended to accept config parser.Config; eventGenerator stores config. Simplified actorName derivation to use tx.TxTo and always use tx.TxFrom as caller for deal messages. isPublishStorageDeals simplified to accept only txType.
Miner sector address handling
tools/miner/sector.go
KeyAddress extraction switched to string (GetItem[string]); use consolidateAddress; update verifiedAllocationKey and Notify entries in-place; error formatting/logging simplified (string formatting).
Public API / JSON surface
types/... (summary)
Public struct shape and JSON schema changed: field renames, additions, removals, and type adjustments affecting persisted records and downstream consumers.

Sequence Diagram(s)

sequenceDiagram
  participant Chain as Chain Node
  participant Parser as Deals Parser (eventGenerator)
  participant Types as types (DealsProposals/DealsMessages)
  participant Store as Output Sink

  Chain->>Parser: PublishStorageDeals(tx)
  Parser->>Types: Build DealsProposals {..., ActorAddress = tx.From}
  Parser->>Types: Build DealsMessages {Data = tx.TxMetadata}
  Parser->>Store: Emit proposals + messages
Loading
sequenceDiagram
  participant Chain as Chain Node
  participant Parser as Activations Parser (eventGenerator)
  participant Types as types (DealsActivations/DealsSpaceInfo)
  participant Store as Output Sink

  Chain->>Parser: VerifyDealsForActivation/ActivateDeals(tx, dealIDs[])
  Parser->>Types: loop per dealID
  Parser->>Types:   Create DealsActivations {DealID, GroupDealIDs, ActorAddress = tx.From}
  Parser->>Types:   Create DealsSpaceInfo {DealID, GroupDealIDs, ActorAddress = tx.From}
  Parser->>Store: Emit per-deal activations & space info
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dev

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
types/deals.go (1)

25-30: Consider consistent field ordering across structs

The ActorAddress field is placed at different positions relative to other fields across the three structs. For better consistency and maintainability, consider placing it at the same position (e.g., right after Height) in all three structs.

Also applies to: 57-66, 69-74

tools/miner/sector.go (2)

408-409: Use error wrapping for logger messages

While logging errors, the change from %w to %s is less critical since these are just logged and execution continues. However, for consistency and better debugging, consider keeping the wrapped format.

-				eg.logger.Errorf("error parsing client id address: %s", err)
+				eg.logger.Errorf("error parsing client id address: %w", err)
-				eg.logger.Errorf("error consolidating client id address: %s", err)
+				eg.logger.Errorf("error consolidating client id address: %w", err)

Also applies to: 413-414


430-431: Maintain consistent error formatting in logger calls

Similar to the previous comment, consider using wrapped error format for consistency.

-					eg.logger.Errorf("error parsing notify number: %s", err)
+					eg.logger.Errorf("error parsing notify number: %w", err)
-					eg.logger.Errorf("error consolidating address: %s", err)
+					eg.logger.Errorf("error consolidating address: %w", err)

Also applies to: 435-436

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 895a6f3 and ef50fd2.

📒 Files selected for processing (4)
  • tools/deals/activations.go (4 hunks)
  • tools/deals/proposal.go (1 hunks)
  • tools/miner/sector.go (2 hunks)
  • types/deals.go (3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tools/miner/sector.go (1)
tools/common/utils.go (1)
  • GetItem (66-84)
tools/deals/activations.go (1)
tools/tools.go (1)
  • BuildId (30-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: tests
  • GitHub Check: build
  • GitHub Check: checks
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (go)
  • GitHub Check: checks
  • GitHub Check: tests
🔇 Additional comments (4)
tools/deals/proposal.go (1)

176-179: LGTM!

The ActorAddress field is correctly populated from the transaction sender.

tools/deals/activations.go (3)

91-101: ActorAddress correctly added for pre-V3 flow

The ActorAddress field is properly populated from the transaction sender for the pre-V3 network version.


151-162: Good improvement for unique ID generation in NV3+ flow

The ID construction now includes the sector index i alongside the full dealIDs array, ensuring unique identifiers for each sector/deal group. This prevents ID collisions when processing multiple sectors.


183-192: ActorAddress consistently populated across activation types

The ActorAddress field is correctly populated from tx.TxFrom for both DealsActivations and DealsSpaceInfo records.

Also applies to: 202-212

Comment thread tools/miner/sector.go
* fix: panic for slice access

* fix: pieceActivationManifest consolidation

* fix: add actor_address to all structs

* fix: separate dealIDs for deal-space

* fix: pass config

* fix: standardize fields and fix actor address
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🔭 Outside diff range comments (2)
tools/deals/deals.go (2)

60-71: Handle address parse failure by skipping the transaction.

On address parse error you log but still proceed to resolve actor name with an invalid/zero-value address. This creates noisy errors and unnecessary work. Bail out early.

Apply this diff:

-		addr, err := address.NewFromString(tx.TxTo)
-		if err != nil {
-			eg.logger.Errorf("could not parse address. Err: %s", err)
-		}
+		addr, err := address.NewFromString(tx.TxTo)
+		if err != nil {
+			eg.logger.Errorf("could not parse address. Err: %v", err)
+			continue
+		}

114-122: Missing BatchActivateDeals in deals-state filter blocks events.

You added parsing for BatchActivateDeals in activations.go, but messages of this type will be skipped here because they’re not recognized as deals-state messages. Add it to the allowlist.

Apply this diff:

 	switch {
 	case strings.EqualFold(txType, parser.MethodPublishStorageDeals),
 		strings.EqualFold(txType, parser.MethodPublishStorageDealsExported),
 		strings.EqualFold(txType, parser.MethodVerifyDealsForActivation),
 		strings.EqualFold(txType, parser.MethodActivateDeals),
+		strings.EqualFold(txType, parser.MethodBatchActivateDeals),
 		strings.EqualFold(txType, parser.MethodSettleDealPaymentsExported),
 		strings.EqualFold(txType, parser.MethodSectorContentChanged):
 		return true
 	}
🧹 Nitpick comments (4)
types/deals.go (1)

81-82: Consider improving the comment clarity

The comment about SpaceAsWeight could be clearer about when to apply the division operation.

-	// SpaceAsWeight is true if the deal space is expressed as a weight. Retrieve the deal space by dividing the VerifiedDealSpace/NonVerifiedDealSpace by the DealDuration for each dealId in the DealIDs slice.
+	// SpaceAsWeight indicates whether the deal space values are expressed as weights (space * duration).
+	// When true, retrieve the actual deal space by dividing VerifiedDealSpace/NonVerifiedDealSpace by the DealDuration.
tools/deals/proposal.go (1)

177-190: Consider extracting repeated consolidation logic

The address consolidation logic for provider and client addresses follows the same pattern. Consider extracting this into a helper method to reduce duplication.

 		if eg.config.ConsolidateRobustAddress {
-			consolidatedProviderAddress, err := eg.consolidateAddress(providerAddress)
-			if err != nil {
-				eg.logger.Errorf("error consolidating provider address: %s", err.Error())
-			} else {
-				providerAddress = consolidatedProviderAddress
-			}
-			consolidatedClientAddress, err := eg.consolidateAddress(clientAddress)
-			if err != nil {
-				eg.logger.Errorf("error consolidating client address: %s", err.Error())
-			} else {
-				clientAddress = consolidatedClientAddress
-			}
+			providerAddress = eg.consolidateAddressWithLogging(providerAddress, "provider")
+			clientAddress = eg.consolidateAddressWithLogging(clientAddress, "client")
 		}

Add this helper method:

func (eg *eventGenerator) consolidateAddressWithLogging(addr string, addrType string) string {
	consolidated, err := eg.consolidateAddress(addr)
	if err != nil {
		eg.logger.Errorf("error consolidating %s address: %s", addrType, err.Error())
		return addr
	}
	return consolidated
}
tools/deals/deals.go (2)

33-33: Config field is stored but unused here.

If config isn’t used yet by this component, consider adding a brief comment on intended usage or defer adding the struct field until needed to avoid confusion.


77-79: Name nit: clarify the meaning of the caller address.

To avoid confusion between the “market actor” (tx.To) and the “caller” (tx.From), consider renaming the local variable.

-		// the address that calls the market actor
-		actorAddress := tx.TxFrom
-		dealMessage, err := eg.createDealMessage(tx, tipsetCid, actorAddress)
+		// caller address that invoked the market actor
+		callerAddress := tx.TxFrom
+		dealMessage, err := eg.createDealMessage(tx, tipsetCid, callerAddress)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between ef50fd2 and 5e4497d.

📒 Files selected for processing (7)
  • parser/v1/parser.go (2 hunks)
  • parser/v2/parser.go (2 hunks)
  • tools/deals/activations.go (4 hunks)
  • tools/deals/deals.go (3 hunks)
  • tools/deals/messages.go (1 hunks)
  • tools/deals/proposal.go (3 hunks)
  • types/deals.go (3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
tools/deals/proposal.go (1)
actors/address.go (1)
  • ConsolidateToRobustAddress (16-43)
tools/deals/deals.go (2)
parser/helper/helpers.go (1)
  • Helper (140-147)
metrics/client.go (1)
  • MetricsClient (12-15)
parser/v1/parser.go (4)
tools/deals/deals.go (1)
  • NewEventGenerator (36-44)
tools/miner/miner.go (1)
  • NewEventGenerator (35-42)
tools/multisig/multisig.go (1)
  • NewEventGenerator (66-73)
logger/logger.go (1)
  • GetSafeLogger (7-17)
parser/v2/parser.go (4)
tools/deals/deals.go (1)
  • NewEventGenerator (36-44)
tools/miner/miner.go (1)
  • NewEventGenerator (35-42)
tools/multisig/multisig.go (1)
  • NewEventGenerator (66-73)
logger/logger.go (1)
  • GetSafeLogger (7-17)
tools/deals/activations.go (2)
types/deals.go (1)
  • DealsSpaceInfo (68-85)
tools/tools.go (1)
  • BuildId (30-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: tests
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (go)
  • GitHub Check: tests
🔇 Additional comments (8)
types/deals.go (2)

26-29: LGTM! Well-structured API enhancement for per-deal attribution

The addition of ActorAddress field to DealsProposals is a thoughtful enhancement that enables proper attribution of deals to specific actors. The field ordering and JSON tag follow Go conventions.


69-74: Good structural refactoring for per-deal granularity

The transition from aggregated DealIDs []uint64 to per-deal structure (DealID uint64 with GroupDealIDs []uint64) provides better granularity and flexibility. The addition of ActorAddress maintains consistency with other structs in this file.

tools/deals/messages.go (1)

28-28: Field rename correctly implemented

The field rename from Value to Data is properly implemented, maintaining consistency with the type definition changes in types/deals.go.

parser/v1/parser.go (1)

73-73: Signature change correctly implemented with proper parameter passing

The NewEventGenerator calls have been properly updated to include both the networkName and config parameters, maintaining consistency across both parser initialization methods.

Also applies to: 89-89

parser/v2/parser.go (1)

75-75: Consistent implementation of the updated event generator signature

The deals event generator initialization correctly passes both networkName and config parameters, maintaining consistency with the v1 parser implementation.

Also applies to: 94-94

tools/deals/proposal.go (3)

8-9: New imports properly added for address handling

The imports for go-address and actors packages are correctly added to support the new address consolidation functionality.


194-194: Actor attribution correctly implemented

The ActorAddress field is properly populated with tx.TxFrom, enabling accurate attribution of deals to the transaction sender.


218-228: Well-structured address consolidation helper

The consolidateAddress method is cleanly implemented with proper error handling and clear error messages.

Comment thread tools/deals/activations.go
Comment thread types/deals.go
* fix: deals message filter

* fix: types for db insertion
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

♻️ Duplicate comments (2)
types/deals.go (1)

20-20: Breaking API change: Field renamed from Value to Data.

This is a breaking change to the public API. Any consumers of the DealsMessages struct will need to update their code to use the new field name Data instead of Value.

tools/deals/activations.go (1)

91-104: Set SpaceAsWeight flag for pre-V3 records.

The fields NonVerifiedDealSpace and VerifiedDealSpace are being populated with weight values (from nonVerifiedDealWeight and verifiedDealWeight), but SpaceAsWeight is not set to true. This will cause downstream consumers to misinterpret the units as bytes instead of weight.

Apply this diff to fix the issue:

 		for _, dealID := range dealIDs {
 			dealSpaceInfo = append(dealSpaceInfo, &types.DealsSpaceInfo{
 				ID:                   tools.BuildId(tx.TxCid, tx.TxFrom, tx.TxTo, fmt.Sprint(tx.Height), tx.TxType, fmt.Sprint(dealID)),
 				Height:               tx.Height,
 				ActorAddress:         tx.TxFrom,
 				TxCid:                tx.TxCid,
 				DealID:               dealID,
 				GroupDealIDs:         dealIDs,
 				NonVerifiedDealSpace: nonVerifiedDealWeight.Uint64(),
 				VerifiedDealSpace:    verifiedDealWeight.Uint64(),
+				SpaceAsWeight:        true,
 				ActionType:           tx.TxType,
 				TxTimestamp:          tx.TxTimestamp,
 			})
 		}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 5e4497d and 1ac9d01.

📒 Files selected for processing (4)
  • tools/deals/activations.go (4 hunks)
  • tools/deals/deals.go (5 hunks)
  • tools/deals/proposal.go (4 hunks)
  • types/deals.go (3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tools/deals/deals.go (2)
parser/helper/helpers.go (1)
  • Helper (140-147)
metrics/client.go (1)
  • MetricsClient (12-15)
tools/deals/proposal.go (4)
tools/version_mapping.go (2)
  • V14 (75-75)
  • MainnetNetwork (21-21)
types/deals.go (1)
  • DealsProposals (24-54)
tools/tools.go (1)
  • BuildId (30-41)
actors/address.go (1)
  • ConsolidateToRobustAddress (16-43)
tools/deals/activations.go (2)
types/deals.go (1)
  • DealsSpaceInfo (67-84)
tools/tools.go (1)
  • BuildId (30-41)
🔇 Additional comments (11)
tools/deals/deals.go (4)

33-42: LGTM! Config integration follows proper dependency injection pattern.

The addition of the config field and its initialization through the constructor maintains proper encapsulation and follows dependency injection best practices.


77-79: Actor address now consistently uses TxFrom.

The change to always use tx.TxFrom as the actor address for deal messages is consistent with the PR's goal of per-deal actor attribution. This ensures the caller of the market actor is properly tracked.


127-135: Simplified method signature improves maintainability.

The removal of the actorName parameter from isPublishStorageDeals simplifies the method signature since it only checks the transaction type. The implementation correctly handles both standard and exported method names.


60-71: No downstream impact detected for actor name derivation change

We verified:

  • No other code paths still call GetActorNameFromAddress with tx.TxFrom (rg -A5 'GetActorNameFromAddress.*TxFrom' returned nothing).
  • Existing tests in tools/miner/miner_test.go and actors/tests/testutil.go mock GetActorNameFromAddress with mock.Anything, so they don’t assume a specific input field.
  • Transactions in tests populate both TxFrom and TxTo, but only the mocked return value is asserted, so behavior remains unchanged.

Since there are no residual dependencies on the old conditional logic and tests pass as-is, no further changes are required.

tools/deals/proposal.go (3)

178-191: Robust address consolidation implementation looks good.

The optional consolidation logic with best-effort error handling is well-structured. Errors are logged but don't break the flow, which is appropriate for address consolidation.


195-195: Per-deal actor attribution properly implemented.

The ActorAddress field is correctly populated with tx.TxFrom, enabling proper tracking of the transaction sender for each deal proposal.


219-229: Address consolidation helper properly implemented.

The consolidateAddress method correctly handles address parsing and consolidation with appropriate error handling. The use of RobustAddressBestEffort config flag provides flexibility.

types/deals.go (1)

27-27: LGTM! ActorAddress field addition enables per-deal attribution.

The addition of ActorAddress to DealsProposals properly supports the PR's goal of per-deal actor attribution.

tools/deals/activations.go (3)

154-168: LGTM! SpaceAsWeight correctly set for V3+ records.

The SpaceAsWeight flag is properly set to true for network versions V3 and above, correctly indicating that the space values represent weights rather than raw bytes.


189-198: Per-deal activation records properly implemented.

The addition of ActorAddress field populated from tx.TxFrom correctly implements per-deal actor attribution for deal activations.


208-221: LGTM! Per-deal space info correctly structured.

The per-deal DealsSpaceInfo records are properly created with individual DealID and GroupDealIDs containing all deal IDs from the sector, supporting the transition from aggregated to per-deal records.

Comment thread tools/deals/proposal.go Outdated
Comment thread types/deals.go Outdated
Comment thread types/deals.go
Comment thread types/deals.go Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🔭 Outside diff range comments (2)
tools/deals/proposal.go (2)

77-98: Pre-NV14 (mainnet) mapping can mis-associate deals; prefer ValidDeals bitfield when present and validate lengths

For mainnet < NV14, mapping indices 0..len(IDs)-1 to params assumes no invalid deals in between. If any input proposals were dropped, you’ll pair the wrong proposal with a returned ID. Also, for NV14+/non-mainnet, add a guard to ensure len(validDealIndices) matches len(dealIDs), or you risk out-of-bounds access later.

Apply this refactor:

  • On the mainnet < NV14 branch, attempt to use ValidDeals (optional) if present.
  • In the bitfield branch, validate that counts match.
  • Fix error messages to say “valid deals” (not “verified”).
-	// From NV0 - NV13 the verified deals are in PublishStorageDealsReturn.IDs
-	// This only applies to mainnet. In calibration the verified deals are in PublishStorageDealsReturn.ValidDeals as a bitfield in all versions.
-	if version.NodeVersion() < tools.V14.NodeVersion() && eg.network == tools.MainnetNetwork {
-		for i, id := range dealIDs {
-			// #nosec G115
-			validDealIndexToDealID[uint64(i)] = id
-		}
-	} else {
-		// From >= NV14 the verified deals are in PublishStorageDealsReturn.ValidDeals as a bitfield
-		validDeals, err := common.GetIntegerSlice[int](ret, KeyValidDeals, false)
-		if err != nil {
-			return nil, fmt.Errorf("error parsing verified deal ids: %w", err)
-		}
-		validDealIndices, err := common.JsonEncodedBitfieldToIDs(validDeals)
-		if err != nil {
-			return nil, fmt.Errorf("error parsing verified deal ids: %w", err)
-		}
-
-		for i, idx := range validDealIndices {
-			validDealIndexToDealID[idx] = dealIDs[i]
-		}
-	}
+	// From NV0 - NV13 (mainnet), many nodes returned only IDs without the ValidDeals bitfield.
+	// Prefer ValidDeals if present (optional), otherwise fall back to positional mapping.
+	if version.NodeVersion() < tools.V14.NodeVersion() && eg.network == tools.MainnetNetwork {
+		if validDealsOpt, err := common.GetIntegerSlice[int](ret, KeyValidDeals, true); err == nil && len(validDealsOpt) > 0 {
+			indices, err := common.JsonEncodedBitfieldToIDs(validDealsOpt)
+			if err != nil {
+				return nil, fmt.Errorf("error parsing valid deal indices: %w", err)
+			}
+			if len(indices) != len(dealIDs) {
+				return nil, fmt.Errorf("mismatch: valid indices (%d) != returned IDs (%d)", len(indices), len(dealIDs))
+			}
+			for i, idx := range indices {
+				validDealIndexToDealID[idx] = dealIDs[i]
+			}
+		} else {
+			for i, id := range dealIDs {
+				// #nosec G115
+				validDealIndexToDealID[uint64(i)] = id
+			}
+		}
+	} else {
+		// From >= NV14 or on calibration, valid deals are in PublishStorageDealsReturn.ValidDeals as a bitfield
+		validDeals, err := common.GetIntegerSlice[int](ret, KeyValidDeals, false)
+		if err != nil {
+			return nil, fmt.Errorf("error parsing valid deals bitfield: %w", err)
+		}
+		validDealIndices, err := common.JsonEncodedBitfieldToIDs(validDeals)
+		if err != nil {
+			return nil, fmt.Errorf("error decoding valid deal indices: %w", err)
+		}
+		if len(validDealIndices) != len(dealIDs) {
+			return nil, fmt.Errorf("mismatch: valid indices (%d) != returned IDs (%d)", len(validDealIndices), len(dealIDs))
+		}
+		for i, idx := range validDealIndices {
+			validDealIndexToDealID[idx] = dealIDs[i]
+		}
+	}

104-106: Compile-time bug: uint64 used as slice index

idx is uint64 (map key). Slice indices must be int. Add bounds check and convert to int to avoid panic and fix compilation.

-	for idx, dealID := range validDealIndexToDealID {
-		deal := deals[idx]
+	for idx, dealID := range validDealIndexToDealID {
+		idxInt := int(idx)
+		if idxInt < 0 || idxInt >= len(deals) {
+			return nil, fmt.Errorf("deal index %d out of range for input deals len=%d", idx, len(deals))
+		}
+		deal := deals[idxInt]
♻️ Duplicate comments (2)
tools/deals/activations.go (1)

91-104: Pre-V3 VerifyDealsForActivation: set SpaceAsWeight=true (units are weights, not bytes)

You’re assigning DealWeight/VerifiedDealWeight into fields named “...DealSpace” for pre-V3, but SpaceAsWeight isn’t set. Downstream consumers will misinterpret units.

 		for _, dealID := range dealIDs {
 			dealSpaceInfo = append(dealSpaceInfo, &types.DealsSpaceInfo{
 				ID:                   tools.BuildId(tx.TxCid, tx.TxFrom, tx.TxTo, fmt.Sprint(tx.Height), tx.TxType, fmt.Sprint(dealID)),
 				Height:               tx.Height,
 				ActorAddress:         tx.TxFrom,
 				TxCid:                tx.TxCid,
 				DealID:               dealID,
 				GroupDealIDs:         dealIDs,
 				NonVerifiedDealSpace: nonVerifiedDealWeight,
 				VerifiedDealSpace:    verifiedDealWeight,
+				SpaceAsWeight:        true,
 				ActionType:           tx.TxType,
 				TxTimestamp:          tx.TxTimestamp,
 			})
 		}
types/deals.go (1)

21-21: Breaking API change: DealsMessages.Value → Data; update docs and versioning

Renaming Value to Data breaks consumers. Ensure docs and examples reflect this, add a clear release note, and bump the module’s major version to follow semver.

🧹 Nitpick comments (1)
types/deals.go (1)

68-85: Doc comment still references DealIDs slice; update for per-deal model

The SpaceAsWeight comment mentions “DealIDs slice” which no longer exists (now DealID + GroupDealIDs). Update wording to avoid confusion.

-	// SpaceAsWeight is true if the deal space is expressed as a weight. Retrieve the deal space by dividing the VerifiedDealSpace/NonVerifiedDealSpace by the DealDuration for each dealId in the DealIDs slice.
+	// SpaceAsWeight is true if the deal space is expressed as a space*time weight.
+	// When true, derive the space for a given deal by dividing the VerifiedDealSpace/NonVerifiedDealSpace by the deal duration.
+	// Use DealID for this record, or iterate GroupDealIDs when interpreting sector-level context.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 1ac9d01 and b04b916.

📒 Files selected for processing (3)
  • tools/deals/activations.go (4 hunks)
  • tools/deals/proposal.go (5 hunks)
  • types/deals.go (3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tools/deals/proposal.go (4)
tools/version_mapping.go (2)
  • V14 (75-75)
  • MainnetNetwork (21-21)
types/deals.go (1)
  • DealsProposals (25-55)
tools/tools.go (1)
  • BuildId (30-41)
actors/address.go (1)
  • ConsolidateToRobustAddress (16-43)
tools/deals/activations.go (2)
types/deals.go (1)
  • DealsSpaceInfo (68-85)
tools/tools.go (1)
  • BuildId (30-41)
🔇 Additional comments (5)
tools/deals/proposal.go (1)

193-213: BigInt adoption for PricePerEpoch and collaterals looks correct

Switching to *big.Int here aligns with the updated types and prevents overflow/truncation risk previously present with Uint64 conversions.

tools/deals/activations.go (3)

154-168: LGTM: NV3..max per-deal entries with SpaceAsWeight=true

Per-deal emission and explicit SpaceAsWeight flag correctly model V3+ semantics.


188-199: LGTM: Per-deal DealsActivations entries with ActorAddress

Accurate per-deal activations, including ActorAddress and stable ID composition.


208-221: LGTM: Per-deal DealsSpaceInfo for ActivateDeals return

Correct per-deal breakdown with GroupDealIDs and big.Int space fields.

types/deals.go (1)

49-53: LGTM: BigInt for price and collaterals with UInt256 gorm annotations

This resolves previous overflow risks and aligns storage with on-chain ranges.

Comment thread tools/deals/proposal.go
Comment thread tools/deals/proposal.go
@ziscky ziscky merged commit 6a8434c into main Aug 14, 2025
12 checks passed
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