Skip to content

New Release#372

Merged
ziscky merged 1 commit into
mainfrom
dev
May 14, 2025
Merged

New Release#372
ziscky merged 1 commit into
mainfrom
dev

Conversation

@ziscky
Copy link
Copy Markdown
Contributor

@ziscky ziscky commented May 14, 2025

🔗 zboto Link

* feat: add miner metrics

* feat: enhance metrics

* feat: log error on parse failure

* account: check for empty return

* eam: allow robust address

* market: activateDeals edge case

* miner: GetSectorSize implementation

* verifreg: make claimedSpace a slice

* fix: lint

* test: update

* fix: review comments
Copy link
Copy Markdown

@windsurf-bot windsurf-bot Bot left a comment

Choose a reason for hiding this comment

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

Other comments: 0 Blocking, 3 Optional, 4 Nit, 1 FYI
  • Optional: actors/v2/account/generic.go (19-26) Optional: May your code be showered with wisdom and peace. 🙏✨

    I notice that when rawReturn is empty, the return value r is no longer added to the metadata map. This creates an inconsistency with the previous behavior where r (even in its zero/default state) was always added to the metadata.

    If this change in behavior is intentional, it's fine. But if you want to maintain consistent behavior, consider adding r to the metadata even when rawReturn is empty.

    Go in peace, and may your commits be ever harmonious. ✝️

  • Optional: actors/v2/miner/sector.go (127-128) Optional: May your code be showered with wisdom and peace. 🙏✨

    The GetSectorSize function no longer validates the network and height parameters against supported versions, unlike all other functions in this file. This could lead to unexpected behavior if called with unsupported heights.

    Consider adding version validation similar to other functions:

    func (*Miner) GetSectorSize(network string, height int64, rawReturn []byte) (map[string]interface{}, error) {
    	version := tools.VersionFromHeight(network, height)
    	// Add version validation if needed
    	return parseGeneric(rawReturn, nil, false, &types.GetSectorSizeReturn{}, &abi.EmptyValue{}, parser.ParamsKey)
    }
    

    Go in peace, and may your commits be ever harmonious. ✝️

  • Optional: tools/miner/miner.go (98-103) Optional: May your code be showered with wisdom and peace. 🙏✨

    I notice the error returned by the metrics update methods is being ignored with the underscore. Consider logging these errors to help diagnose any metrics reporting issues.

    	for txType, total := range parsedInfoTotalsByTxType {
    		if err := eg.metrics.UpdateProcessedMinerInfoTotalMetric(txType, total); err != nil {
    			eg.logger.Errorf("failed to update miner info metrics: %v", err)
    		}
    	}
    	for txType, total := range parsedSectorsTotalsByTxType {
    		if err := eg.metrics.UpdateProcessedMinerSectorsTotalMetric(txType, total); err != nil {
    			eg.logger.Errorf("failed to update miner sectors metrics: %v", err)
    		}
    	}
    

    Go in peace, and may your commits be ever harmonious. ✝️

  • Nit: actors/v2/market/types/batchActivateDeals.go (24-24) Nit: There's a small typo in this comment that could cause confusion. The word 'deals' appears twice in the sentence.\n\n```suggestion\n// some sector deals DO NOT include the sector number\n```
  • Nit: actors/v2/verifiedRegistry/types/claimAllocations.go (68-68) Nit: May your code be showered with wisdom and peace. 🙏✨

    The comment on line 68 mentions 'SectorClaimSummary' which doesn't appear to be used in the implementation. Consider updating the comment to accurately reflect that ClaimedSpace is now a slice of big.Int values.

    	// t.ClaimedSpace ([]big.Int) - can be either a single value or an array of values
    

    Go in peace, and may your commits be ever harmonious. ✝️

  • Nit: tools/miner/metrics.go (61-63) Nit: May your code be showered with wisdom and peace. 🙏✨

    The comment for processedMinerSectorsTotalMetric appears to be copied from the processedMinerInfoTotalMetric comment. Consider updating it to specifically describe sectors rather than generic 'events'.

    	// Register metrics for processed miner sectors
    	// Usage: Increment counter when sectors have been processed
    	processedMinerSectorsTotalMetric = metrics.Metric{
    

    Go in peace, and may your commits be ever harmonious. ✝️

  • Nit: parser_test.go (2048-2048) Nit: May your code be showered with wisdom and peace. 🙏✨

    The comment explains that we're skipping equality checks for ClaimAllocations because its return structure changed, but it would be helpful to add more details about the specific change (e.g., 'changed from X to Y to support slices'). This would make the exception more understandable for future developers maintaining this code.

    Go in peace, and may your commits be ever harmonious. ✝️

  • FYI: actors/v2/eam/eam.go (76-76) FYI: May your code be showered with wisdom and peace. 🙏✨

    I notice that newEamCreate has been changed from a standalone function to a method of the Eam struct. This change requires updating all existing calls to this function throughout the codebase to use the receiver syntax.

    Go in peace, and may your commits be ever harmonious. ✝️

Comment thread actors/v2/eam/eam.go
Comment thread actors/v2/eam/create.go
Comment thread actors/v2/miner/miner.go
@ziscky ziscky requested a review from emmanuelm41 May 14, 2025 14:11
@ziscky ziscky merged commit 664b370 into main May 14, 2025
11 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