[refactor] Remove errors from subaccounts keeper functions that don't return errors #678
Conversation
| m.AssetPositions = append([]*AssetPosition{usdcAssetPosition}, m.AssetPositions...) | ||
| func (m *Subaccount) SetUsdcAssetPosition(newUsdcPosition *big.Int) { | ||
| if m == nil { | ||
| return |
There was a problem hiding this comment.
Should we panic / error log if this ever happens?
It doesn't seem like this function should silently fail if called with a nil pointer.
There was a problem hiding this comment.
I think this was meant to be a convenience method for tests only. I don't think there is a reason to use this instead of UpdateAssetPositions. wdyt?
WalkthroughThe changes primarily focus on simplifying the error handling and return values in the subaccounts module. The functions Changes
TipsChat with CodeRabbit Bot (
|
| @@ -106,7 +106,6 @@ func UpdatePerpetualPositions( | |||
| perpIdToFundingIndex map[uint32]dtypes.SerializableInt, | |||
| ) ( | |||
| success bool, | |||
| err error, | |||
There was a problem hiding this comment.
This function doesn't return an error anymore, so removed it.
| AssetId: assettypes.AssetUsdc.Id, | ||
| } | ||
| m.AssetPositions = append([]*AssetPosition{usdcAssetPosition}, m.AssetPositions...) | ||
| func (m *Subaccount) SetUsdcAssetPosition(newUsdcPosition *big.Int) { |
There was a problem hiding this comment.
This function also doesn't return an error anymore, so removed it + refactored it to early return.
Let me know if anyone would prefer I remove the refactor logic to early return. This function has good test coverage though so I felt like the change is safe.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (4)
- protocol/x/subaccounts/keeper/subaccount.go (2 hunks)
- protocol/x/subaccounts/keeper/subaccount_helper.go (4 hunks)
- protocol/x/subaccounts/types/subaccount.go (1 hunks)
- protocol/x/subaccounts/types/subaccount_test.go (2 hunks)
Files skipped from review due to trivial changes (2)
- protocol/x/subaccounts/keeper/subaccount_helper.go
- protocol/x/subaccounts/types/subaccount.go
Additional comments (Suppressed): 3
protocol/x/subaccounts/types/subaccount_test.go (2)
205-210: The removal of
expectedErrorfrom the test case struct indicates that the functionSetUsdcAssetPositionno longer returns an error. Ensure that this change is intentional and that all error handling is properly managed upstream or within the function itself.310-316: The error check after calling
SetUsdcAssetPositionhas been removed. This is consistent with the change in the function's signature to no longer return an error. However, it's important to ensure that the function is now handling all possible error scenarios internally, or that these scenarios are being managed upstream.protocol/x/subaccounts/keeper/subaccount.go (1)
- 433-437: The
SetUsdcAssetPositionmethod no longer returns an error. This change simplifies the code but also removes the ability to handle errors that might occur when setting the USDC asset position. Ensure that theSetUsdcAssetPositionmethod is designed to handle all possible errors internally, or consider adding error handling back to this method if necessary.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (2)
- protocol/x/subaccounts/keeper/subaccount.go (2 hunks)
- protocol/x/subaccounts/keeper/subaccount_helper.go (3 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/subaccounts/keeper/subaccount_helper.go
Additional comments (Suppressed): 2
protocol/x/subaccounts/keeper/subaccount.go (2)
286-292: The error handling has been removed from the
UpdatePerpetualPositionsandUpdateAssetPositionsfunctions. Ensure that these functions are now designed to never fail, or that any errors they encounter are handled internally. If not, this could lead to silent failures and unexpected behavior.430-430: The error handling has been removed from the
SetUsdcAssetPositionfunction. As with the previous comment, ensure that this function is now designed to never fail, or that any errors it encounters are handled internally. If not, this could lead to silent failures and unexpected behavior.
| UpdatePerpetualPositions( | ||
| settledUpdates, | ||
| perpIdToFundingIndex, | ||
| ) | ||
| if !success || err != nil { | ||
| return success, successPerUpdate, err | ||
| } | ||
|
|
||
| // Apply the updates to asset positions. | ||
| success, err = UpdateAssetPositions(settledUpdates) | ||
| if !success || err != nil { | ||
| return success, successPerUpdate, err | ||
| } | ||
| UpdateAssetPositions(settledUpdates) |
There was a problem hiding this comment.
The error handling has been removed from the UpdatePerpetualPositions and UpdateAssetPositions functions. This could potentially lead to silent failures if these functions encounter an error. Consider adding error handling back in to ensure that any issues are properly caught and handled.
- UpdatePerpetualPositions(
- settledUpdates,
- perpIdToFundingIndex,
- )
- UpdateAssetPositions(settledUpdates)
+ success, err = UpdatePerpetualPositions(
+ settledUpdates,
+ perpIdToFundingIndex,
+ )
+ if !success || err != nil {
+ return success, successPerUpdate, err
+ }
+ success, err = UpdateAssetPositions(settledUpdates)
+ if !success || err != nil {
+ return success, successPerUpdate, err
+ }| return types.Subaccount{}, nil, err | ||
| } | ||
| // TODO(CLOB-993): Remove this function and use `UpdateAssetPositions` instead. | ||
| newSubaccount.SetUsdcAssetPosition(newUsdcPosition) |
There was a problem hiding this comment.
The SetUsdcAssetPosition method no longer returns an error. This could potentially lead to silent failures if this method encounters an error. Consider adding error handling back in to ensure that any issues are properly caught and handled.
- newSubaccount.SetUsdcAssetPosition(newUsdcPosition)
+ err = newSubaccount.SetUsdcAssetPosition(newUsdcPosition)
+ if err != nil {
+ return types.Subaccount{}, nil, err
+ }
Changelist
Remove errors from subaccounts keeper functions that don't return errors.
This is part of a larger subaccounts keeper refactor to simplify the code by removing returned errors when invariants are broken, and instead panicing earlier.
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.