Conversation
* feat: deals parsing template * feat: add parsing of storage deals proposals * feat: add dealIds to miner sector data * feat: conditional version * feat: impl. ParseDealsEvent on parserV1 * refactor: move common methods to diff. pkg * test: update utils tests * fix: lint * refactor: move common methods to diff. pkg * feat: parse deal space/weight info and activations * fix: refactor struct names * fix: use correct return type * fix: remove unused metrics * fix: parsing bugs * fix: parsing bugs * fix: activation parsing bugs * fix: lint * fix: test case * feat: add comments on version switches * test: use hel1 light node * test: use zur1 full node
* fix: skip special case check for block cids * fix: lint
WalkthroughThis change introduces comprehensive support for parsing and generating deal-related events from blockchain transactions. It adds new utility packages and event generators for deals, implements parsing logic for deal proposals and activations, and defines new types for structured deal event data. Existing miner utilities are refactored to use the new common utilities, and related tests are updated or removed accordingly. The parser's interface and implementation are extended to expose deal event parsing, and a new constant for batch deal activation is added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser
participant DealsEventGenerator
participant Helper/Logger/Metrics
Client->>Parser: ParseDealsEvents(ctx, txs, tipsetCid, tipsetKey)
Parser->>DealsEventGenerator: GenerateDealsEvents(ctx, txs, tipsetCid, tipsetKey)
DealsEventGenerator->>Helper/Logger/Metrics: (Resolve actor info, log, update metrics)
DealsEventGenerator-->>Parser: DealsEvents (messages, proposals, activations, space info)
Parser-->>Client: DealsEvents
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
tools/common/utils_test.go (2)
16-34: Add error condition test cases.The test covers basic functionality but lacks coverage for error conditions. Consider adding test cases for:
- Invalid string format that can't be converted to big.Int
- Missing key when canBeNil is false
- Nil params when canBeNil is false
{name: "test 3", params: map[string]interface{}{key: ""}, canBeNil: true, want: nil}, + {name: "invalid format", params: map[string]interface{}{key: "invalid"}, canBeNil: false, expectError: true}, + {name: "missing key", params: map[string]interface{}{}, canBeNil: false, expectError: true},
36-54: Add error condition test cases.Similar to TestGetBigInt, this test lacks coverage for error conditions. Consider adding test cases for:
- Wrong type input (e.g., string instead of float64)
- Missing key when canBeNil is false
types/deals.go (1)
15-23: Consider adding documentation for the Value field.The
Valuefield's purpose is unclear from its name alone. Consider adding a comment to clarify what type of value this represents (e.g., transaction value, deal value, etc.).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (19)
actors/v2/tools.go(2 hunks)factory.go(2 hunks)parser/constants.go(1 hunks)parser/v1/parser.go(5 hunks)parser/v2/parser.go(5 hunks)parser_test.go(1 hunks)tools/common/utils.go(1 hunks)tools/common/utils_test.go(1 hunks)tools/deals/activations.go(1 hunks)tools/deals/deals.go(1 hunks)tools/deals/messages.go(1 hunks)tools/deals/metrics.go(1 hunks)tools/deals/proposal.go(1 hunks)tools/miner/info.go(12 hunks)tools/miner/sector.go(14 hunks)tools/miner/sector_test.go(0 hunks)tools/miner/utils.go(0 hunks)tools/miner/utils_test.go(0 hunks)types/deals.go(1 hunks)
💤 Files with no reviewable changes (3)
- tools/miner/sector_test.go
- tools/miner/utils_test.go
- tools/miner/utils.go
🧰 Additional context used
🧬 Code Graph Analysis (9)
factory.go (2)
types/transaction.go (1)
Transaction(14-46)types/deals.go (1)
DealsEvents(8-13)
tools/common/utils_test.go (1)
tools/common/utils.go (4)
GetBigInt(13-26)GetInteger(28-35)GetIntegerSlice(37-49)JsonEncodedBitfieldToIDs(120-147)
tools/deals/messages.go (5)
types/transaction.go (1)
Transaction(14-46)types/deals.go (1)
DealsMessages(15-23)tools/version_mapping.go (2)
VersionFromHeight(317-373)V20(96-96)parser/constants.go (2)
MethodActivateDeals(154-154)MethodBatchActivateDeals(155-155)tools/tools.go (1)
BuildId(30-41)
actors/v2/tools.go (1)
parser/constants.go (1)
MethodAwardBlockReward(75-75)
tools/deals/deals.go (5)
types/transaction.go (1)
Transaction(14-46)types/deals.go (5)
DealsEvents(8-13)DealsMessages(15-23)DealsProposals(25-54)DealsActivations(56-64)DealsSpaceInfo(66-81)parser/helper/helpers.go (1)
Helper(140-147)metrics/client.go (1)
MetricsClient(12-15)parser/constants.go (8)
MethodUpdateClaimedPower(80-80)TotalFeeOp(10-10)MethodPublishStorageDeals(149-149)MethodPublishStorageDealsExported(150-150)MethodVerifyDealsForActivation(153-153)MethodActivateDeals(154-154)MethodSettleDealPaymentsExported(170-170)MethodSectorContentChanged(171-171)
parser/v2/parser.go (6)
tools/deals/deals.go (2)
EventGenerator(22-24)NewEventGenerator(35-42)logger/logger.go (1)
GetSafeLogger(7-17)parser/v1/parser.go (1)
Parser(42-56)factory.go (1)
Parser(48-60)types/transaction.go (1)
Transaction(14-46)types/deals.go (1)
DealsEvents(8-13)
tools/miner/sector.go (4)
tools/common/utils.go (5)
GetItem(66-84)GetInteger(28-35)GetIntegerSlice(37-49)GetSlice(86-113)JsonEncodedBitfieldToIDs(120-147)tools/deals/proposal.go (2)
KeyParams(30-30)KeyDealIDs(17-17)tools/deals/activations.go (1)
KeySectors(16-16)types/miner.go (1)
MinerSectorEvent(19-28)
parser/v1/parser.go (5)
tools/deals/deals.go (2)
EventGenerator(22-24)NewEventGenerator(35-42)logger/logger.go (1)
GetSafeLogger(7-17)factory.go (1)
Parser(48-60)parser/v2/parser.go (1)
Parser(44-58)types/deals.go (1)
DealsEvents(8-13)
tools/deals/metrics.go (1)
metrics/client.go (2)
MetricsClient(12-15)Metric(17-22)
⏰ 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). (6)
- GitHub Check: Analyze (go)
- GitHub Check: tests
- GitHub Check: build
- GitHub Check: checks
- GitHub Check: Analyze (go)
- GitHub Check: tests
🔇 Additional comments (24)
parser_test.go (1)
58-58: Good infrastructure improvement.Updating from a "light" node to a "stable" node endpoint is a sensible change for tests, as stable nodes provide better reliability and consistency for test execution.
parser/constants.go (1)
155-155: Well-structured constant addition.The new
MethodBatchActivateDealsconstant follows the established naming convention and is logically placed after the relatedMethodActivateDealsconstant. This addition supports batch processing of deal activations for enhanced performance in newer network versions.tools/miner/info.go (1)
9-9: Excellent refactoring to centralize utility functions.The migration from local
getItemandgetSlicehelper functions to the centralizedcommon.GetItemandcommon.GetSlicefunctions is a positive change that:
- Reduces code duplication across the codebase
- Improves maintainability by centralizing common functionality
- Maintains consistent error handling patterns
- Follows good software engineering practices
The changes are systematically applied across all parsing functions while preserving the existing logic and error handling.
Also applies to: 62-69, 100-103, 135-150, 190-196, 236-242, 277-277, 304-323, 314-338
actors/v2/tools.go (1)
111-138: Review: Verify the intentional drop of “empty‐block” special casesIt looks like v2/tools.go now only treats
MethodAwardBlockRewardas a special case; all the other transaction types that used to immediately return an emptyblockCidwill now fall through and hit the default lookup intipset.BlockMessages. Please confirm:
- That
ExtendedTipSet.BlockMessagesis indeed populated (or intentionally left empty) for the previously‐skipped tx types.- That no downstream code or tests rely on those types returning
""unconditionally.If this change is by design (i.e. you want default lookup to handle them, or simply return
""if they never appear in any block), then no further action is needed. Otherwise, restore the excluded tx types or add any missing special cases back.factory.go (1)
56-56: LGTM! Well-implemented interface extension.The new
ParseDealsEventsmethod follows the established pattern of other event parsing methods. The interface definition is clean and the implementation correctly delegates toparserV2, maintaining consistency with other event parsing functionality.Also applies to: 247-249
tools/miner/sector.go (1)
11-11: Excellent refactoring with valuable feature addition.This refactoring successfully centralizes utility functions from local implementations to the
tools/commonpackage, improving code maintainability. The addition ofKeyDealIDssupport in pre-commit events is a valuable enhancement that allows tracking of deal associations with sectors.The consistent replacement of functions like
getInteger→common.GetIntegerandjsonEncodedBitfieldToSectorNumbers→common.JsonEncodedBitfieldToIDsfollows good practices for code deduplication.Also applies to: 21-21, 129-132, 142-142
parser/v2/parser.go (1)
31-31: Perfect integration following established patterns.The deals event parsing integration is implemented consistently with existing event generators. The field placement, constructor initialization, and method implementation all follow the established patterns used by
multisigEventGeneratorandminerEventGenerator.The delegation to
dealsEventGenerator.GenerateDealsEventsmaintains the separation of concerns and keeps the parser focused on orchestration rather than implementation details.Also applies to: 53-53, 75-75, 94-94, 299-301
tools/deals/metrics.go (1)
45-52: Avoid modifying metric definitions in placeThe method modifies the input metrics by appending to their Labels slice directly. This could cause issues if the same metric definitions are reused elsewhere, leading to duplicate labels being appended.
func (c *dealsMetricsClient) registerModuleMetrics(metrics ...metrics.Metric) { commonLabels := []string{parserModule} for i := range metrics { - metrics[i].Labels = append(metrics[i].Labels, commonLabels...) + // Create a copy of the metric to avoid modifying the original + metricCopy := metrics[i] + metricCopy.Labels = append(metricCopy.Labels, commonLabels...) + metrics[i] = metricCopy } c.RegisterCustomMetrics(metrics...) }Likely an incorrect or invalid review comment.
types/deals.go (4)
8-13: LGTM!The
DealsEventsstruct is well-structured as a container for different types of deal events, using pointer slices appropriately for memory efficiency.
25-54: LGTM!The
DealsProposalsstruct is well-designed with appropriate types and excellent documentation for complex fields. The use of*big.Intfor collateral values is the correct choice for handling large blockchain values.
56-64: LGTM!The
DealsActivationsstruct is well-structured with appropriate types for representing deal activation events.
66-81: LGTM!The
DealsSpaceInfostruct is excellently documented with clear explanations of the space and weight fields. The use of*big.Intfor space values and theSpaceAsWeightflag with its detailed comment make the struct's purpose and usage clear.tools/deals/activations.go (7)
15-23: LGTM!Well-organized constants for map keys used throughout the parsing logic.
25-65: LGTM!The
createDealActivationsfunction correctly handles different transaction types and appropriately manages optional return values for different network versions.
168-243: LGTM!The
parseActivateDealsfunction elegantly handles version-specific parsing with a well-structured nested function approach. The version branching and error handling are comprehensive.
245-262: LGTM!Clean helper function with appropriate use of named returns and proper error handling.
264-275: LGTM!Simple and focused helper function with proper error handling.
277-300: LGTM!Well-implemented function that correctly handles optional fields and properly accumulates
big.Intvalues for verified deal space calculation.
302-311: LGTM!Clean helper function that clearly identifies deal activation transaction types.
parser/v1/parser.go (5)
30-30: LGTM!The import alias follows the established pattern used for other tool packages.
49-49: LGTM!The field addition follows the established pattern and is positioned appropriately within the struct.
73-73: LGTM!The deals event generator initialization follows the established pattern with appropriate parameters.
89-89: LGTM!The deals event generator initialization in the v2 constructor is consistent with the pattern and uses the correct network parameter.
230-232: LGTM!The method implementation correctly delegates to the deals event generator and matches the interface signature. The parameter naming follows established conventions.
| for i := 0; i < len(sectorDeals); i++ { | ||
| dealIDs, nonVerifiedDealWeight, verifiedDealWeight, err := eg.getCommonVerifyDealForActivationFields(sectorDeals[i], sectorWeights[i]) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| dealSpaceInfo = append(dealSpaceInfo, &types.DealsSpaceInfo{ | ||
| ID: tools.BuildId(tx.TxCid, tx.TxFrom, tx.TxTo, fmt.Sprint(tx.Height), tx.TxType, fmt.Sprint(dealIDs[i])), | ||
| Height: tx.Height, | ||
| TxCid: tx.TxCid, | ||
| DealIDs: dealIDs, | ||
| NonVerifiedDealSpace: nonVerifiedDealWeight, | ||
| VerifiedDealSpace: verifiedDealWeight, | ||
| SpaceAsWeight: true, | ||
| ActionType: tx.TxType, | ||
| TxTimestamp: tx.TxTimestamp, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Fix array indexing error in DealSpaceInfo ID generation.
The loop iterates through all sectorDeals, but line 151 uses dealIDs[i] which will cause an index out of bounds error. Should use the entire dealIDs array or iterate through dealIDs separately.
Apply this diff to fix the issue:
- ID: tools.BuildId(tx.TxCid, tx.TxFrom, tx.TxTo, fmt.Sprint(tx.Height), tx.TxType, fmt.Sprint(dealIDs[i])),
+ ID: tools.BuildId(tx.TxCid, tx.TxFrom, tx.TxTo, fmt.Sprint(tx.Height), tx.TxType, fmt.Sprint(dealIDs)),🤖 Prompt for AI Agents
In tools/deals/activations.go around lines 144 to 161, the code incorrectly uses
dealIDs[i] inside the loop over sectorDeals, causing an index out of bounds
error. To fix this, do not index dealIDs with i; instead, use the entire dealIDs
slice as is when building the ID or iterate separately over dealIDs if
individual IDs are needed. Adjust the code so that the ID generation correctly
handles all dealIDs without indexing errors.
2 participants
🔗 zboto Link
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores