Conversation
* docs: add comments * test: use -race * docs: add comments * fix: review comments * test: fix race * fix: cache setup mutex * fix: zcache init mutex * test: rm parallel * fix: retry logic * test: rm parallel
There was a problem hiding this comment.
Other comments: 1 Blocking, 3 Optional, 1 Nit, 2 FYI
-
Blocking: actors/cache/actors_cache.go (51-53)
Blocking: May your code be showered with wisdom and peace. 🙏✨
The mutex
setupMuis declared as a local variable within theSetupActorsCachefunction, which means it will be created anew with each function call and won't protect against concurrent invocations of this function.If the intention is to make this function thread-safe, consider using a package-level mutex instead:
var setupMu sync.Mutex func SetupActorsCache(dataSource common.DataSource, logger *logger.Logger, metrics metrics.MetricsClient, backoff backoff.BackOff) (*ActorsCache, error) { setupMu.Lock() defer setupMu.Unlock()Go in peace, and may your commits be ever harmonious. ✝️
-
Optional: actors/v2/ethaccount/parse.go (165-167)
Optional: May your code be showered with wisdom and peace. 🙏✨
The
ValueTransfermethod is being added but currently returns an empty map. If this is intentional as a placeholder for future implementation, please add a TODO comment. Otherwise, consider implementing proper parameter parsing similar to other methods.Go in peace, and may your commits be ever harmonious. ✝️
-
Optional: tools/tools.go (57-57)
Optional: May your code be showered with wisdom and peace. 🙏✨
The deprecation notice is helpful, but it would be more beneficial to provide additional context about when this function will be removed and any differences between the old and new implementations that users should be aware of when migrating.
// Deprecated: Use v2/tools.GetBlockCidFromMsgCid instead. This function will be removed in a future release.Go in peace, and may your commits be ever harmonious. ✝️
-
Optional: actors/cache/impl/onchain.go (104-104)
Optional: May your code be showered with wisdom and peace. 🙏✨
I notice the function name has been changed from
StateLookupWithRetrytoNodeApiCallWithRetry, which is a blessed improvement as it better describes the generic nature of the function that can retry any node API call, not just state lookups.Please ensure that the function implementation itself (not visible in this diff) has been renamed to match. If this function is used elsewhere in the codebase, those references should also be updated.
Go in peace, and may your commits be ever harmonious. ✝️
-
Nit: actors/v2/tools.go (49-49)
Nit: May your code be showered with wisdom and peace. 🙏✨
There should be a space after the double slash in this comment for consistency with other comments in the file.
// https://github.com/filecoin-project/builtin-actors/blob/8fdbdec5e3f46b60ba0132d90533783a44c5961f/actors/ethaccount/src/lib.rs#L54Go in peace, and may your commits be ever harmonious. ✝️
-
FYI: actors/v2/miner/miner.go (53-55)
FYI: May your code be showered with wisdom and peace. 🙏✨
I notice the variable name has been corrected from 'movePartionsMethodNum' to 'movePartitionsMethodNum'. This correction brings harmony to your codebase by ensuring consistent naming. 🔧
Go in peace, and may your commits be ever harmonious. ✝️
-
FYI: parser/common.go (120-120)
FYI: May your code be showered with wisdom and peace. 🙏✨
The renaming from
StateLookupWithRetrytoNodeApiCallWithRetryis a blessed improvement that better reflects the function's purpose. This makes the code more self-documenting and easier to understand.Go in peace, and may your commits be ever harmonious. ✝️
🔗 zboto Link