New Release#455
Conversation
* fix: change actor_address to miner_address * fix: change value to data
WalkthroughThe changes rename two fields in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant EventGenerator
participant MinerInfo
Caller->>EventGenerator: createMinerInfo(tx, tipsetCid, actorAddress)
alt tx is AwardBlockReward
EventGenerator->>MinerInfo: parseAwardBlockReward(tx, tipsetCid)
else tx is Constructor
EventGenerator->>MinerInfo: parseConstructor(tx, tipsetCid, actorAddress)
else tx is ChangeWorkerAddress
EventGenerator->>MinerInfo: parseChangeWorkerAddress(tx, tipsetCid, actorAddress)
else tx is ChangeMultiaddrs
EventGenerator->>MinerInfo: parseChangeMultiaddrs(tx, tipsetCid, actorAddress)
else tx is ChangeBeneficiary
EventGenerator->>MinerInfo: parseChangeBeneficiary(tx, tipsetCid, actorAddress)
else tx is ChangeOwnerAddress
EventGenerator->>MinerInfo: parseChangeOwnerAddress(tx, tipsetCid, actorAddress)
end
MinerInfo-->>EventGenerator: MinerInfo instance with consolidated addresses
EventGenerator-->>Caller: Return MinerInfo with updated fields
✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
* feat: refactor GetActorNameFromAddress to GetActorInfoFromAddress * feat: create GetActorNameFromAddress fn with protocol fallback * feat: use GetActorNameFromAddress in msig propose * fix: use consts
* feat: consolidate addresses in miner data * fix: build * feat: consolidate addresses in sector messages * fix: add empty map checks
Bumps [github.com/zondax/golem](https://github.com/zondax/golem) from 0.23.0 to 0.26.0. - [Release notes](https://github.com/zondax/golem/releases) - [Commits](Zondax/golem@v0.23.0...v0.26.0) --- updated-dependencies: - dependency-name: github.com/zondax/golem dependency-version: 0.26.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github.com/go-viper/mapstructure/v2](https://github.com/go-viper/mapstructure) from 2.2.1 to 2.3.0. - [Release notes](https://github.com/go-viper/mapstructure/releases) - [Changelog](https://github.com/go-viper/mapstructure/blob/main/CHANGELOG.md) - [Commits](go-viper/mapstructure@v2.2.1...v2.3.0) --- updated-dependencies: - dependency-name: github.com/go-viper/mapstructure/v2 dependency-version: 2.3.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Emmanuel <emmanuelm41@gmail.com> Co-authored-by: Eric Mokaya <4112301+ziscky@users.noreply.github.com> Co-authored-by: Emmanuel <e.m.m.a.n.u.e.l.m.41@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Sara Elzayat <st.elzayat@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tools/miner/sector.go (1)
281-296: Consider consolidating error messages.The address consolidation logic is correctly implemented. However, all error messages use the same generic text.
- return nil, fmt.Errorf("error parsing sealer id: %w", err) + return nil, fmt.Errorf("error converting sealer id to address: %w", err)- return nil, fmt.Errorf("error consolidating sealer id: %w", err) + return nil, fmt.Errorf("error consolidating sealer address to robust format: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
actors/v1/actor.go(1 hunks)actors/v2/init/init.go(1 hunks)actors/v2/multisig/propose.go(1 hunks)factory.go(1 hunks)go.mod(5 hunks)parser/helper/helpers.go(7 hunks)parser/v1/parser.go(5 hunks)parser/v2/parser.go(3 hunks)tools/miner/info.go(2 hunks)tools/miner/miner.go(2 hunks)tools/miner/miner_test.go(3 hunks)tools/miner/sector.go(5 hunks)tools/multisig/multisig.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- actors/v2/multisig/propose.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/miner/miner_test.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
tools/miner/miner.go (3)
parser/config.go (1)
Config(3-13)parser/helper/helpers.go (1)
Helper(140-147)metrics/client.go (1)
MetricsClient(12-15)
tools/miner/info.go (6)
types/transaction.go (1)
Transaction(14-46)types/miner.go (1)
MinerInfo(9-17)parser/constants.go (6)
MethodAwardBlockReward(75-75)MethodConstructor(48-48)MethodChangeWorkerAddress(101-101)MethodChangeMultiaddrs(108-108)MethodChangeBeneficiary(124-124)MethodChangeOwnerAddress(99-99)tools/tools.go (1)
BuildId(30-41)actors/address.go (1)
ConsolidateToRobustAddress(16-43)tools/miner/sector.go (1)
KeyParams(24-24)
parser/v1/parser.go (3)
tools/miner/miner.go (2)
EventGenerator(22-24)NewEventGenerator(35-42)tools/multisig/multisig.go (2)
EventGenerator(52-54)NewEventGenerator(62-68)logger/logger.go (1)
GetSafeLogger(7-17)
⏰ 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). (8)
- GitHub Check: tests
- GitHub Check: build
- GitHub Check: checks
- GitHub Check: tests
- GitHub Check: build
- GitHub Check: checks
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
🔇 Additional comments (26)
tools/multisig/multisig.go (1)
103-103: LGTM: Consistent with codebase-wide actor info retrieval refactor.The change from
GetActorNameFromAddresstoGetActorInfoFromAddressaligns with the broader refactor to centralize actor information retrieval. The CID return value is appropriately ignored since only the actor name is needed for the multisig actor type check.actors/v1/actor.go (1)
44-44: LGTM: Proper actor info retrieval update.The change to
GetActorInfoFromAddressis consistent with the codebase refactor and correctly ignores the CID return value since only the actor name is needed for the subsequent actor type switch statement.factory.go (1)
528-528: LGTM: Genesis address processing updated consistently.The change to
GetActorInfoFromAddressis appropriate for genesis address processing and follows the consistent pattern across the codebase. The CID is correctly ignored since only the actor name is needed for the address info construction.actors/v2/init/init.go (1)
176-176: LGTM: Enhanced actor info retrieval with both CID and name.This change effectively utilizes both return values from
GetActorInfoFromAddress, providing richer actor information for the init actor processing. This demonstrates the benefit of the refactor by returning bothgotActorCidandgotActorNameinstead of just the name.tools/miner/miner.go (3)
32-32: LGTM: Configuration field added for address consolidation.The addition of the
configfield supports address consolidation features in miner event generation, as indicated by theConsolidateRobustAddressandRobustAddressBestEffortflags in theparser.Configstruct.
35-35: LGTM: Constructor updated to accept configuration.The constructor signature change and field initialization are consistent with the config field addition, enabling configuration-driven miner event generation.
Also applies to: 40-40
68-68: LGTM: Actor info retrieval updated consistently.The change from
GetActorNameFromAddresstoGetActorInfoFromAddressfollows the same pattern as other files in this refactor. The CID is appropriately ignored since only the actor name is needed for the miner actor type check.parser/v2/parser.go (2)
72-72: LGTM!The addition of the
configparameter to the miner event generator constructor is consistent with the initialization pattern used for other components in the parser.Also applies to: 90-90
602-602: LGTM!The migration to
GetActorInfoFromAddressis consistent with the codebase-wide refactoring. The actor CID is appropriately discarded since only the actor name is needed in this context.parser/v1/parser.go (3)
30-30: LGTM!The addition of the miner tools import and the
minerEventGeneratorfield properly extends the v1 parser with miner event generation capabilities, maintaining consistency with the v2 parser structure.Also applies to: 48-48
70-70: LGTM!The miner event generator initialization follows the same pattern as the multisig event generator and correctly passes all required dependencies including the config parameter.
Also applies to: 85-85
514-514: LGTM!The method call update is consistent with the v2 parser implementation and correctly handles the additional return value.
parser/helper/helpers.go (5)
69-71: LGTM!The new constants are well-documented and appropriately named for their purposes.
178-219: LGTM!The new
GetActorAddressInfomethod provides a comprehensive actor information structure with proper error handling and special case handling for zero address actors.
242-278: LGTM!The
GetActorInfoFromAddressmethod is well-designed with:
- Proper handling of special account actors
- Smart retry logic for placeholder actors using on-chain data
- Comprehensive error handling and logging
192-192: LGTM!All method calls have been consistently updated to use
GetActorInfoFromAddresswith proper handling of the additional return values.Also applies to: 223-223, 351-351, 437-437, 449-449
221-240: EAM space address prefix is explicitly definedThe
f410prefix is declared and documented in parser/helper/helpers.go, confirming this assumption:
- File: parser/helper/helpers.go
Line: whereEamSpaceAddressPrefix = "f410"is defined with the comment
// EamSpaceAddressPrefix f410 is the prefix addresses managed by the eam actorSince this constant and its purpose are clearly documented in code, the heuristic for treating
f410…delegated addresses as EVM actors is valid. No further changes required.tools/miner/sector.go (4)
8-8: LGTM!The new imports are necessary for the address consolidation functionality being added.
Also applies to: 11-11
18-35: LGTM!The new constants follow a consistent naming pattern and are well-organized for parsing various miner sector event fields.
307-307: LGTM!The addition of the context parameter and the conditional address consolidation for pieces is well-implemented. The context parameter appears to be added for future use, which is a good forward-thinking design.
Also applies to: 319-331
431-482: Well-structured nested address consolidation!The
consolidatePieceActivationManifestsmethod excellently handles the complex nested structure with:
- Proper error handling at each parsing step
- Performance optimization through slice pre-allocation
- Clear separation of concerns for different field types
tools/miner/info.go (5)
7-8: LGTM!The new imports are necessary for the address parsing and consolidation functionality.
14-24: LGTM!The constants are well-named and organized for parsing various miner transaction metadata fields.
26-54: Excellent refactoring with dedicated parsers!The switch statement provides clean separation of concerns by delegating to specialized parsing methods for each transaction type. The field rename to
MinerAddressis consistent with the type definition changes.
56-90: LGTM!The
parseAwardBlockRewardmethod correctly extracts and consolidates the miner address with proper error handling at each step.
341-378: LGTM for parseChangeOwnerAddress!This method has more appropriate error messages compared to the others. The implementation correctly handles owner address consolidation.
* feat: consolidate signer address * feat: consolidate AddSigner & RemoveSigner * feat: consolidate Constructor signers
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tools/multisig/multisig.go (1)
218-265: Consider refactoring address consolidation logic to reduce duplication.The address consolidation logic is functionally correct but contains significant code duplication between the add/remove signer and constructor cases. Consider extracting common consolidation logic into helper functions.
Example refactor:
+func (eg *eventGenerator) consolidateSignerAddress(signerAddr string) (string, error) { + addr, err := address.NewFromString(signerAddr) + if err != nil { + return "", fmt.Errorf("address.NewFromString(%s): %s", signerAddr, err) + } + return actors.ConsolidateToRobustAddress(addr, eg.helper, eg.logger, eg.config.RobustAddressBestEffort) +} + +func (eg *eventGenerator) consolidateSignerAddresses(signers []string) ([]string, error) { + consolidated := make([]string, 0, len(signers)) + for _, signer := range signers { + robustAddr, err := eg.consolidateSignerAddress(signer) + if err != nil { + return nil, err + } + consolidated = append(consolidated, robustAddr) + } + return consolidated, nil +}This would reduce the duplication and make the code more maintainable.
📜 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 (3)
parser/v1/parser.go(5 hunks)parser/v2/parser.go(3 hunks)tools/multisig/multisig.go(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- parser/v1/parser.go
- parser/v2/parser.go
⏰ 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). (8)
- GitHub Check: checks
- GitHub Check: tests
- GitHub Check: build
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (go)
- GitHub Check: tests
- GitHub Check: checks
🔇 Additional comments (8)
tools/multisig/multisig.go (8)
9-9: LGTM: Import added for address consolidation functionality.The new import is necessary for the address consolidation logic implemented later in the file.
37-38: LGTM: Constants properly defined for metadata fields.The new constants are well-named and improve code maintainability by avoiding magic strings.
63-63: LGTM: Configuration support properly integrated.The config field addition and constructor updates are well-implemented and follow consistent patterns.
Also applies to: 66-66, 71-71
95-98: LGTM: Improved error handling.The error handling for
createProposalis now properly implemented instead of being silently ignored.
132-132: LGTM: Method signature updated to return error.The
createProposalmethod now properly returns an error, improving error handling consistency.Also applies to: 148-148
310-320: LGTM: Helper functions are simple and correct.The helper functions
isAddOrRemoveSignerandisConstructorare well-implemented and improve code readability.
353-353: LGTM: Helpful comment about address type.The comment clarifies that the address is already robust, which is useful context for developers.
111-111: AllGetActorInfoFromAddresscalls match the updated signature
A repository-wide search confirms every invocation unpacks three return values(cid.Cid, string, error), with the second value consistently treated as the actor name. No mismatched arity or lingering calls to the old method remain.
2 participants
🔗 zboto Link
Summary by CodeRabbit
Refactor
Tests