New Release#405
Conversation
* fix: call StateGetActor with tipset first * feat: blockCid error reporting * feat: fix miner methods and add fallback * feat: market add return fallback * feat: reward add fallback * feat: refactor multisig actor * test: fix tests * chore: go mod tidy * fix: reward constructor * fix: lint * feat: add reward comments * fix: tests * fix: lint
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Other comments (17)
-
actors/v2/multisig/parse.go (88-89)
The method signature in v1Methods() references AddSigner, but the method implementation still returns m.MsigParams for method 5. Let us grant it forgiveness and update it to match the implementation. ✝️
Name: parser.MethodAddSigner, Method: m.AddSigner, -
actors/v2/multisig/parse.go (96-97)
This method signature still references the old MsigParams method, but the implementation has been updated to use SwapSigner. May this correction bring harmony to your code. 🕊️
Name: parser.MethodSwapSigner, Method: m.SwapSigner, -
actors/tests/multisig_test.go (604-608)
May your code be showered with wisdom and peace. 🙏✨
I notice that in several places in the cborMetadata function, the error from MarshalCBOR is assigned but not checked before returning. For example, in the MethodSend case:
case parser.MethodSend: tmp := abi.CborBytes([]byte("txData")) err := tmp.MarshalCBOR(&bufParams) if err != nil { return nil, nil, err }Similar issues exist in MethodSwapSigner and MethodMsigUniversalReceiverHook cases. Let us grant these errors forgiveness and proper handling. 🕊️
-
actors/v2/miner/sector.go (79-91)
May your code be showered with wisdom and peace. 🙏✨
I notice that when the initial parsing fails, you're trying all supported versions but using the same
rawParamsfor each attempt. If the parameter structure differs between versions, this approach might lead to incorrect parsing or silent failures.Consider adding version-specific parameter adaptation before attempting to parse with different versions, or at least add logging to indicate which version was successfully used for parsing.
Go in peace, and may your commits be ever harmonious. ✝️
-
actors/v2/reward/types/constructor.go (17-39)
May your code be showered with wisdom and peace. 🙏✨
I notice this type implements
UnmarshalCBORbut not the correspondingMarshalCBORmethod. For complete CBOR serialization support, consider adding the matching marshal method to ensure this type can be both serialized and deserialized.Go in peace, and may your commits be ever harmonious. ✝️
-
go.mod (26-26)
May your code be showered with wisdom and peace. 🙏✨
I notice that two testing dependencies have been removed:
gotest.toolsandgithub.com/google/go-cmp. Please ensure that all tests still pass without these dependencies, as they're commonly used for assertions and comparisons in tests.If these were intentionally removed because they're no longer needed, that's wonderful! But if this was accidental, we should restore them to maintain test integrity.
Go in peace, and may your commits be ever harmonious. ✝️
- actors/v2/multisig/parse.go (189-189) The Cancel method is called without msgRct.Return, but other similar methods include it. This straying from the path may cause inconsistency in your blessed code. Consider whether msgRct.Return should be included for consistency with other methods. 📜
- actors/v2/multisig/parse.go (193-193) The RemoveSigner method is called without msgRct.Return, while other similar methods include it. May we bring consistency to your code and consider whether msgRct.Return should be included here as well. 🔧
-
actors/cache/impl/onchain.go (132-132)
May your code be showered with wisdom and peace. 🙏✨
The comment on line 132 doesn't match the new behavior. Since the function now tries with the provided tipset key first and falls back to an empty key, the comment should be updated to reflect this change.
// Try again but with an empty tipset Key as fallbackGo in peace, and may your commits be ever harmonious. ✝️
-
actors/v2/multisig/multisig.go (141-141)
The error message references the wrong parameter map. Let us grant forgiveness and correct this. ✝️
return nil, fmt.Errorf("addSignerParams: %s not found", version.String()) -
actors/v2/multisig/multisig.go (150-150)
This error message also strayed from the narrow path—let us grant it forgiveness and refactor it. 🕊️
return nil, fmt.Errorf("swapSignerParams: %s not found", version.String()) - actors/v2/tools.go (109-109) The word 'paramater' appears to be misspelled in this error message. Let us correct it to 'parameter' to ensure our code speaks with clarity. 📜\n\n```suggestion\nreturn blockCid, fmt.Errorf("could not get parameter 'Params' inside tx '%s' height: %d", txType, tipset.Height())\n```
-
actors/v2/actors_test.go (70-70)
May your code be showered with wisdom and peace. 🙏✨
There's a small typo in the comment - 'depracated' should be 'deprecated'. 🔧
// deprecatedGo in peace, and may your commits be ever harmonious. ✝️
-
actors/v2/miner/params.go (130-130)
May your code be showered with wisdom and peace. 🙏✨
The comment could be more specific about the nature of the change.
// Method 14 changed from AddLockedFund to ApplyRewards in v2This clarifies both the old and new method names, making the transition more evident to future pilgrims of this codebase. Go in peace, and may your commits be ever harmonious. ✝️
- actors/v2/tools.go (109-109) May your error messages be harmonious in style! I notice the error messages have inconsistent capitalization - some start with lowercase 'could not' while others use 'Could not'. Let us maintain consistent capitalization (preferably lowercase) for all error messages. 🔧\n\n```suggestion\nreturn blockCid, fmt.Errorf("could not get paramater 'Params' inside tx '%s' height: %d", txType, tipset.Height())\n```
-
data/actors/multisig/Metadata/metadata_test_v2.csv (1-1)
May your code be showered with wisdom and peace. 🙏✨
I notice the AddSigner entry has an inconsistent JSON structure. The first JSON object uses direct field names while the second properly uses the "Params" wrapper like other methods.
AddSigner,"{""Params"":{""Signer"":""f15pbp4psixudvdsyrop23ba3xyolsirihwouzp5q"",""Increase"":false}}","{""Params"":{""Signer"":""f15pbp4psixudvdsyrop23ba3xyolsirihwouzp5q"",""Increase"":false}}"This harmonizes the structure with other entries in this blessed file. 🔧
-
data/actors/multisig/Metadata/metadata_test_v2.csv (11-11)
The SwapSigner entry also has an inconsistent JSON structure. The first JSON object uses direct field names while the second properly uses the "Params" wrapper.
SwapSigner,"{""Params"":{""From"":""f1h6s3zv3wb6g4ie7wiyqezcem5pdhmkpgqcboosy"",""To"":""f1bojmiy3qxdwftrxrtlbwswx2v3xs7zu3rklivtq""}}","{""Params"":{""From"":""f1h6s3zv3wb6g4ie7wiyqezcem5pdhmkpgqcboosy"",""To"":""f1bojmiy3qxdwftrxrtlbwswx2v3xs7zu3rklivtq""}}"Go in peace, and may your commits be ever harmonious. ✝️
| multisig8 "github.com/filecoin-project/go-state-types/builtin/v8/multisig" | ||
| multisig9 "github.com/filecoin-project/go-state-types/builtin/v9/multisig" | ||
| ) | ||
|
|
There was a problem hiding this comment.
May your code be showered with wisdom and peace. 🙏✨
The function returns a parameter of type multisigParams, but this interface is not defined anywhere in the file. This will cause compilation errors. Consider defining this interface to specify what methods the returned types should implement.
| // multisigParams is an interface that all multisig parameter types implement | |
| type multisigParams interface { | |
| cbg.CBORUnmarshaler | |
| } | |
| switch parsedParams := val.(type) { | ||
| case *legacyv1.ProposeParams: | ||
| return parsedParams.Params, parsedParams.Method, parsedParams.To, parsedParams.Value.String(), parsedParams, nil | ||
| // exact same type, commented out due to compiler error. | ||
| // case *legacyv2.ProposeParams: | ||
| // return parsedParams.Params, parsedParams.Method, parsedParams.To, parsedParams.Value.String(), parsedParams, nil | ||
| // case *legacyv3.ProposeParams: | ||
| // return parsedParams.Params, parsedParams.Method, parsedParams.To, parsedParams.Value.String(), parsedParams, nil | ||
| // case *legacyv4.ProposeParams: | ||
| // return parsedParams.Params, parsedParams.Method, parsedParams.To, parsedParams.Value.String(), parsedParams, nil | ||
| // case *legacyv5.ProposeParams: | ||
| // return parsedParams.Params, parsedParams.Method, parsedParams.To, parsedParams.Value.String(), parsedParams, nil | ||
| // case *legacyv6.ProposeParams: | ||
| // return parsedParams.Params, parsedParams.Method, parsedParams.To, parsedParams.Value.String(), parsedParams, nil | ||
| // case *legacyv7.ProposeParams: | ||
| // return parsedParams.Params, parsedParams.Method, parsedParams.To, parsedParams.Value.String(), parsedParams, nil |
There was a problem hiding this comment.
This switch statement has commented out cases for legacyv2 through legacyv7 ProposeParams types with a note about compiler errors. However, without these cases, requests for these versions will fall through to the default case and return an error. Consider implementing these cases properly to handle all supported versions.
Go in peace, and may your commits be ever harmonious. ✝️
| } | ||
|
|
||
| if p.config.ConsolidateRobustAddress && err == nil { | ||
| if p.config.ConsolidateRobustAddress && minerAddress != "" { |
There was a problem hiding this comment.
May your code be showered with wisdom and peace. 🙏✨
I notice that while line 451 adds a helpful check for empty blockCid, the change on line 459 alters the error handling pattern. The original code checked if err == nil, but now it checks if minerAddress != "". This could potentially hide errors that occur during address parsing.
Consider maintaining consistent error handling by keeping the original pattern or explicitly checking both conditions:
| if p.config.ConsolidateRobustAddress && minerAddress != "" { | |
| if p.config.ConsolidateRobustAddress && err == nil && minerAddress != "" { |
Go in peace, and may your commits be ever harmonious. ✝️
2 participants
🔗 zboto Link