Conversation
* feat: make actorsCache implement IActorsCache * fix: sector events errors * test: set up mocks * test: miner events tests * fix: actorname comparison * test: fix global var * chore: go mod tidy
* feat: only set valid addresses * feat: add f080 as a system actor * feat: add mainnet multisig system actors * test: add calibration test * test: add calibration genesis tests * feat: InvokedActor fallback * fix: parse mainnet actor names
There was a problem hiding this comment.
Other comments (20)
-
actors/v1/power.go (113-113)
May your code be showered with wisdom and peace. 🙏✨
I notice the method name has changed from
StoreAddressInfoAddresstoStoreAddressInfo. Please ensure this method exists in the ActorsCache interface and that all other calls to this method have been updated throughout the codebase for consistency.Go in peace, and may your commits be ever harmonious. ✝️
- actors/v2/power/power.go (200-200) The method name has been changed from `StoreAddressInfoAddress` to `StoreAddressInfo`. Please verify that this new method exists in the ActorsCache implementation and accepts the same parameter type to avoid potential runtime errors.
-
actors/cache/types.go (20-20)
May your code be showered with wisdom and peace. 🙏✨
I notice the
GetActorCodemethod signature has been modified to include a newonChainOnlyboolean parameter. This is a breaking change that will require updates to all implementations of theIActorsCacheinterface. Please ensure all implementations are updated accordingly to maintain interface compliance.Go in peace, and may your commits be ever harmonious. ✝️
-
actors/v1/eam.go (112-112)
May your code be showered with wisdom and peace. 🙏✨
I notice the method name has changed from
StoreAddressInfoAddresstoStoreAddressInfoin three places. Please verify that this method exists in the ActorsCache implementation to avoid runtime errors.Go in peace, and may your commits be ever harmonious. ✝️
-
go.mod (3-3)
May your code be showered with wisdom and peace. 🙏✨
I notice the Go version is specified as
go 1.23.7, but this appears to be an invalid version number. The latest Go releases are in the 1.22.x range as of mid-2024. This could cause build issues or confusion.go 1.21.7Go in peace, and may your commits be ever harmonious. ✝️
-
actors/v1/init.go (82-82)
May your code be showered with wisdom and peace. 🙏✨
I notice the method name has changed from
StoreAddressInfoAddresstoStoreAddressInfo. Please ensure this method exists in the actors cache implementation and that all other calls to this method throughout the codebase have been updated for consistency.Go in peace, and may your commits be ever harmonious. ✝️
-
actors/v2/tools.go (32-32)
May your code be showered with wisdom and peace. 🙏✨
Changing from equality checks (
actorName == manifest.PlaceholderKey) to substring checks (strings.Contains(actorName, manifest.PlaceholderKey)) is a significant behavior change that could lead to false positives.For example, if an actor name is
"NotPlaceholder"but contains the string"Placeholder", it would now match when it shouldn't. Consider whether this change is intentional and if a more precise check like string prefix/suffix or exact matching would be more appropriate.Go in peace, and may your commits be ever harmonious. ✝️
-
actors/v2/tools.go (83-83)
May your code be showered with wisdom and peace. 🙏✨
There appears to be an inconsistency in the codebase. While the GetMethodName function now uses
strings.Contains()for actor name checks, the ActorMethods function (line 83) still uses equality checks:if actorName == manifest.EthAccountKey || actorName == manifest.PlaceholderKey {
This inconsistency could lead to bugs where GetMethodName identifies an actor by substring, but ActorMethods doesn't recognize it because it requires an exact match. These two functions should use consistent approaches for actor name identification. 🔧
Go in peace, and may your commits be ever harmonious. ✝️
-
actors/v2/eam/generic.go (87-87)
May your code be showered with wisdom and peace. 🙏✨
I notice the function call has changed from
StoreAddressInfoAddresstoStoreAddressInfo. Please ensure this matches the actual implementation in the actors cache package, as this could lead to runtime errors if the function signature doesn't exist.Go in peace, and may your commits be ever harmonious. ✝️
-
tools/miner/sector.go (296-296)
May your code be showered with wisdom and peace. 🙏✨
I notice you've changed the key used to retrieve sector activations from
KeySectorActivationstoKeySectors. While this appears to be an intentional alignment with the actual data structure, please confirm this is compatible with all existing data formats you're processing. This change could potentially break parsing for older data formats if both keys were used in different contexts.Go in peace, and may your commits be ever harmonious. ✝️
-
go.mod (5-5)
May your code be blessed with consistency. 🙏✨
There appears to be an inconsistency between the
godirective (1.23.7) and thetoolchaindirective (1.24.1). These should typically be aligned to avoid confusion and potential build issues.If you intend to use Go 1.24.1, consider updating both directives to match. 🔧
Go in peace, and may your commits be ever harmonious. ✝️
-
parser/v2/parser.go (566-569)
When there's an error getting the method name in `getActorAndMethodName`, you're setting `txType = parser.UnknownStr` but not logging the error. This differs from the error handling in `getTxType` where similar errors are logged. Consider adding error logging here for consistency:
txType, err = actorsV2.GetMethodName(ctx, trace.Msg.Method, actorName, int64(tipset.Height()), p.network, p.helper, p.logger) if err != nil { p.logger.Errorf("Error when trying to get method name in tx cid'%s' using v2: %v", mainMsgCid.String(), err) txType = parser.UnknownStr } -
actors/cache/impl/zcache.go (167-167)
May your code be showered with wisdom and peace. 🙏✨
I notice the GetActorCode method has been modified to accept a third parameter that's immediately discarded (marked with underscore). Consider adding a comment explaining the purpose of this parameter for future maintainers.
func (m *ZCache) GetActorCode(address address.Address, key filTypes.TipSetKey, _ bool /* explain parameter purpose here */) (string, error) {Go in peace, and may your commits be ever harmonious. ✝️
-
actors/cache/impl/zcache.go (344-346)
May your code be showered with wisdom and peace. 🙏✨
The new ClearBadAddressCache method has an empty implementation. Consider adding a more descriptive comment explaining why this method exists in the interface but has no implementation in this particular cache type.
func (m *ZCache) ClearBadAddressCache() { // This implementation doesn't need to clear any bad address cache // as it doesn't maintain one, unlike other cache implementations }Go in peace, and may your commits be ever harmonious. ✝️
-
tools/miner/sector.go (379-379)
May your code be showered with wisdom and peace. 🙏✨
This change from
KeySectorstoKeySectorNumbersrepresents another key modification in the data structure access pattern. Please ensure this change is consistent with the actual structure of the data being processed, as it could affect the parsing of existing records.It would be beneficial to document why these key changes were necessary - perhaps in a comment or the PR description - to help future developers understand the reasoning behind these modifications. 🔧
Go in peace, and may your commits be ever harmonious. ✝️
-
actors/address.go (12-12)
May your code be showered with wisdom and peace. 🙏✨
This change from concrete type
*cache.ActorsCacheto interfacecache.IActorsCacheis a blessed improvement toward dependency inversion! 🔧 This will make testing easier and improve flexibility.Have you verified that all callers of this function have been updated to accommodate this interface change?
Go in peace, and may your commits be ever harmonious. ✝️
- parser/v2/parser.go (555-561) The error logging in `getActorAndMethodName` is inconsistent. For the first error case (getting actor name from address), you're using `logger.Warnf`, but for the second error case (getting actor name from CID), you're using `logger.Errorf`. Consider using the same log level for similar error scenarios to maintain consistency in your logs.
-
.mockery.yaml (19-19)
May your code be showered with wisdom and peace. 🙏✨
The file is missing a newline at the end. Let us grant it forgiveness and add one to follow standard file formatting practices.
mockname: "IActorsCache"Go in peace, and may your commits be ever harmonious. ✝️
-
data/genesis/calibration_genesis_balances.json (114-114)
May your code be showered with wisdom and peace. 🙏✨
I notice this file is missing a newline at the end. Adding a newline at the end of files is a good practice for Unix/Linux compatibility and helps with certain text processing tools.
}Go in peace, and may your commits be ever harmonious. ✝️
-
actors/cache/impl/onchain.go (163-163)
May your code be showered with wisdom and peace. 🙏✨
There's a typo in the error message: 'unimplimented' should be 'unimplemented'.
return fmt.Errorf("unimplemented")Go in peace, and may your commits be ever harmonious. ✝️
1 file skipped due to size limits:
- tools/mocks/fullnode_mock.go
* feat: check for system exec * fix: initial pledge * fix: withdraw balance * feat: add bock_cid cases * feat: update actorsCache interface * feat: rm bock_cid cases * feat: fallback to block messages * feat: unify address parsing * test: fix tests * feat: withdrawBalance edge case * fix: set raw bytes on failure * test: update test values * fix: address comments
🔗 zboto Link