New Release#460
Conversation
* fix: allow errors in addr consolidation * fix: multisig lenient consolidation
WalkthroughThe changes refactor address consolidation logic across miner and multisig tools by introducing new helper methods in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant eventGenerator
participant Helper/Logger
participant Config
Caller->>eventGenerator: parseXxx(params)
eventGenerator->>eventGenerator: consolidateAddress(addrStr)
eventGenerator->>Helper/Logger: Log error if consolidation fails
eventGenerator->>params: Update params with consolidated address (if successful)
eventGenerator-->>Caller: Return updated params (with logging, not errors)
sequenceDiagram
participant Caller
participant eventGenerator
participant Helper/Logger
participant Config
Caller->>eventGenerator: createMultisigInfo(meta, config)
eventGenerator->>eventGenerator: consolidateAddress(signerAddr)
alt Consolidation succeeds for all
eventGenerator->>meta: Update with consolidated signers
else Any consolidation fails
eventGenerator->>Helper/Logger: Log error, skip update
end
eventGenerator-->>Caller: Return updated meta (no error returned)
✨ 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: 1
🧹 Nitpick comments (1)
tools/miner/sector.go (1)
426-483: Consider usingcontinueinstead ofbreakfor cleaner flowThe current implementation uses
breakstatements when consolidation fails, but still appends the piece toparsedPieces. While the final check ensures original pieces are returned on any failure, this approach processes and stores partial data unnecessarily.Consider either:
- Using
continueto skip failed pieces entirely, or- Only appending to
parsedPieceswhen all consolidations for a piece succeedExample refactor using approach 2:
func (eg *eventGenerator) consolidatePieceActivationManifests(_ context.Context, pieces []map[string]interface{}) ([]map[string]interface{}, error) { parsedPieces := make([]map[string]interface{}, 0, len(pieces)) for _, piece := range pieces { + pieceValid := true verifiedAllocationKey, err := getItem[map[string]interface{}](piece, KeyVerifiedAllocationKey, true) if err != nil { return nil, fmt.Errorf("error parsing verified allocation key: %w", err) } if len(verifiedAllocationKey) > 0 { clientIDAddrStr, err := getItem[uint64](verifiedAllocationKey, KeyAddress, false) if err != nil { eg.logger.Errorf("error parsing client id address: %w", err) - break + pieceValid = false } - consolidatedClientIDAddr, err := eg.consolidateIDAddress(clientIDAddrStr) - if err != nil { - eg.logger.Errorf("error consolidating client id address: %w", err) - break + if pieceValid { + consolidatedClientIDAddr, err := eg.consolidateIDAddress(clientIDAddrStr) + if err != nil { + eg.logger.Errorf("error consolidating client id address: %w", err) + pieceValid = false + } else { + verifiedAllocationKey[KeyAddress] = consolidatedClientIDAddr + piece[KeyVerifiedAllocationKey] = verifiedAllocationKey + } } - verifiedAllocationKey[KeyAddress] = consolidatedClientIDAddr - piece[KeyVerifiedAllocationKey] = verifiedAllocationKey } // Similar pattern for dataActivationNotifications... - parsedPieces = append(parsedPieces, piece) + if pieceValid { + parsedPieces = append(parsedPieces, piece) + } }
📜 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 (4)
tools/miner/info.go(7 hunks)tools/miner/sector.go(3 hunks)tools/miner/utils.go(2 hunks)tools/multisig/multisig.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tools/multisig/multisig.go (1)
actors/address.go (1)
ConsolidateToRobustAddress(16-43)
tools/miner/utils.go (1)
actors/address.go (1)
ConsolidateToRobustAddress(16-43)
tools/miner/info.go (1)
tools/miner/sector.go (1)
KeyParams(22-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). (5)
- GitHub Check: tests
- GitHub Check: checks
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
- GitHub Check: tests
🔇 Additional comments (2)
tools/multisig/multisig.go (1)
218-267: Well-structured error handling for address consolidationThe refactored code properly handles partial failures by:
- Only attempting consolidation when addresses are present
- Logging errors without aborting the entire operation
- Applying updates only when all consolidations succeed
This defensive approach ensures robustness while maintaining data integrity.
tools/miner/info.go (1)
303-355: Clean implementation of constructor address consolidationThe new
consolidateConstructorAddressesmethod effectively centralizes the consolidation logic for constructor parameters. The approach of updating address arrays only when all elements consolidate successfully ensures data consistency.
| func (eg *eventGenerator) consolidateAddress(addrStr string) (string, error) { | ||
| addr, err := address.NewFromString(addrStr) | ||
| if err != nil { | ||
| return "", fmt.Errorf("error parsing id address: %w", err) |
There was a problem hiding this comment.
Fix the incorrect error message
The error message refers to "id address" but this method handles any address type, not just ID addresses.
- return "", fmt.Errorf("error parsing id address: %w", err)
+ return "", fmt.Errorf("error parsing address: %w", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return "", fmt.Errorf("error parsing id address: %w", err) | |
| - return "", fmt.Errorf("error parsing id address: %w", err) | |
| + return "", fmt.Errorf("error parsing address: %w", err) |
🤖 Prompt for AI Agents
In tools/miner/utils.go at line 126, the error message incorrectly specifies "id
address" while the method processes any address type. Update the error message
to a more general term like "address" to accurately reflect the scope of the
method.
2 participants
🔗 zboto Link
Summary by CodeRabbit
Refactor
New Features