fix(avs): resolve more audit findings #220
Conversation
WalkthroughThe pull request introduces extensive modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
@coderabbitai review this pr |
|
@trestinlsd: I will review the PR. ✅ Actions performedReview triggered.
|
trestinlsd
requested review from
MaxMustermann2,
mikebraver,
bwhour,
cloud8little,
TimmyExogenous and
leonz789
and removed request for
MaxMustermann2
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
🧹 Outside diff range and nitpick comments (15)
x/avs/keeper/params.go (1)
105-121: New structs enhance AVS management capabilities.The addition of
TaskResultParamsandOperatorParamsstructs aligns with the PR objectives of addressing audit findings and adding missing interfaces. These new structures provide a more robust way to handle task results and operator parameters, which should improve the overall functionality and maintainability of the AVS system.Consider adding comments to explain the purpose of these structs and their fields, especially if they're part of a public API.
x/avs/keeper/task.go (2)
388-389: LGTM: Consistent case-insensitive retrieval of challenged task informationThe addition of
strings.ToLower()on bothoperatorAddressandtaskContractAddressensures consistent, case-insensitive retrieval of challenged task information. This change maintains consistency with the modifications in other functions.Consider moving the
strings.ToLower()calls to the beginning of the function for consistency with other functions and to ensure all operations use the lowercase versions:func (k *Keeper) GetTaskChallengedInfo(ctx sdk.Context, operatorAddress, taskContractAddress string, taskID uint64) (addr string, err error) { + operatorAddress = strings.ToLower(operatorAddress) + taskContractAddress = strings.ToLower(taskContractAddress) if !common.IsHexAddress(taskContractAddress) { return "", types.ErrInvalidAddr } store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixTaskChallengeResult) - infoKey := assetstype.GetJoinedStoreKey(strings.ToLower(operatorAddress), strings.ToLower(taskContractAddress), + infoKey := assetstype.GetJoinedStoreKey(operatorAddress, taskContractAddress, strconv.FormatUint(taskID, 10)) // ... rest of the function }This change would make the function more consistent with the pattern used in other functions and ensure that all operations within the function use the lowercase versions of the addresses.
Line range hint
1-398: Summary: Consistent case-insensitive address handling implementedThe changes in this file focus on normalizing address strings to lowercase across various functions in the AVS keeper. This modification:
- Improves consistency in address handling throughout the system.
- Aligns with best practices for managing Ethereum addresses.
- Enhances the robustness of the system by preventing case-sensitivity issues.
These changes are well-implemented and consistent across all modified functions. The overall impact is positive, reducing the potential for errors related to address case mismatches.
Consider implementing a utility function for address normalization to further improve code maintainability and ensure consistent handling across the entire codebase. For example:
func NormalizeAddress(address string) string { return strings.ToLower(address) }This function could then be used throughout the codebase, making it easier to modify the normalization logic if needed in the future.
precompiles/avs/abi.json (1)
554-576: LGTM: New functiongetTaskInfoadded, consider adding documentationThe new
getTaskInfofunction has been correctly implemented as aviewfunction. It provides a way to retrieve information about a specific task. However, consider adding documentation to explain the meaning of each element in the returneduint64[]array for better usability.Consider adding a comment explaining the structure of the returned
infoarray, e.g.:/// @return info An array of task information: /// [0]: task status /// [1]: creation timestamp /// [2]: ...precompiles/avs/query.go (1)
144-165: Enhance error message on invalid operator addressIn
IsOperator, consider providing a more descriptive error message ifsdk.AccAddressFromBech32fails, to assist in debugging invalid address formats.param, err := sdk.AccAddressFromBech32(operatorAddr) if err != nil { - return nil, err + return nil, fmt.Errorf("invalid operator address '%s': %v", operatorAddr, err) }precompiles/avs/events.go (1)
261-290: Consistent use of boolean literalsAcross multiple event emission functions, a boolean literal
trueis being passed as an argument toarguments.Pack(). While this might be intentional, consider using a named constant or variable for better readability and maintainability.For example:
const success = true // Then use 'success' in arguments.Pack() packed, err := arguments.Pack( common.HexToAddress(params.Operator), params.Name, success)This approach improves code clarity and allows for easier modifications in the future.
precompiles/avs/IAVSManager.sol (2)
149-149: Update Function Documentation for Modified Return ValuesThe
createTaskfunction now returns an additionaluint64 taskID. The function's documentation should be updated to reflect this change, ensuring that callers are aware of the updated return values and can handle them appropriately.
230-235: Enhance Return Types in Query FunctionsThe
getAVSInfoandgetTaskInfofunctions return basic data types that may not provide comprehensive information to the caller. Consider returning structured data using custom structs to provide detailed and organized information.Define structs for the return types:
struct AVSInfo { string epochIdentifier; // Add other relevant fields as needed } struct TaskDetails { uint64[] info; // Consider expanding this to named fields for clarity } function getAVSInfo( address avsAddr ) external view returns (AVSInfo memory avsInfo); function getTaskInfo( address taskAddr, uint64 taskID ) external view returns (TaskDetails memory taskDetails);By returning structs with clearly defined fields, you enhance the usability of these functions and make the interface more intuitive for developers.
Also applies to: 236-243
x/avs/keeper/keeper.go (1)
443-454: Add validation checks inSubmitTaskResultThe
SubmitTaskResultmethod assigns values toTaskResultInfowithout validation:func (k Keeper) SubmitTaskResult(ctx sdk.Context, params *TaskResultParams) error { result := &types.TaskResultInfo{ TaskId: params.TaskID, OperatorAddress: params.OperatorAddress, TaskContractAddress: params.TaskContractAddress.String(), TaskResponse: params.TaskResponse, BlsSignature: params.BlsSignature, Stage: params.Stage, } return k.SetTaskResultInfo(ctx, params.OperatorAddress, result) }Consider adding validation for critical parameters, such as ensuring
TaskIDandOperatorAddressare valid and not empty, to prevent potential errors downstream.precompiles/avs/query_test.go (2)
449-491: Correct Function Name toTestIsOperatorThe function name
TestIsoperatordoes not follow Go naming conventions. It should beTestIsOperatorwith proper camel casing to improve readability and maintain consistency.Apply this diff to correct the function name:
-func (suite *AVSManagerPrecompileSuite) TestIsoperator() { +func (suite *AVSManagerPrecompileSuite) TestIsOperator() {
492-550: Use Constants for Task ID to Avoid Magic NumbersIn
TestGetTaskInfo, the task IDuint64(3)is used directly. To improve code clarity and maintainability, define a constant for the task ID.Apply this diff to define a constant:
+const testTaskID uint64 = 3 func (suite *AVSManagerPrecompileSuite) TestGetTaskInfo() { method := suite.precompile.Methods[avsManagerPrecompile.MethodGetTaskInfo] taskAddress := "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48" setUp := func() { info := &avstype.TaskInfo{ TaskContractAddress: taskAddress, Name: "test-avstask-01", - TaskId: uint64(3), + TaskId: testTaskID, Hash: []byte("active"), TaskResponsePeriod: 10, StartingEpoch: 5, TaskStatisticalPeriod: 60, TaskTotalPower: sdk.Dec(sdkmath.NewInt(0)), } err := suite.App.AVSManagerKeeper.SetTaskInfo(suite.Ctx, info) suite.NoError(err) } testCases := []avsTestCases{ { "success - existent task", func() []interface{} { setUp() return []interface{}{ common.HexToAddress(taskAddress), - uint64(3), + testTaskID, } }, func(bz []byte) { var out []uint64 err := suite.precompile.UnpackIntoInterface(&out, avsManagerPrecompile.MethodGetTaskInfo, bz) suite.Require().NoError(err, "failed to unpack output", err) suite.Require().Equal([]uint64{5, 10, 60}, out) }, 100000, false, "", }, }precompiles/avs/avs_test.go (2)
821-919: Add negative test cases toTestRegisterOperatorToExocoreCurrently,
TestRegisterOperatorToExocoreonly tests the successful scenario. Adding negative test cases will improve test coverage and ensure robustness.Consider including test cases for:
- Invalid
metaInfovalues (e.g., empty strings or overly long inputs).- Unauthorized callers attempting to register an operator.
- Handling of unexpected errors during registration.
921-1026: Remove or document commented-out code inTestOperatorSubmitTaskThe entire
TestOperatorSubmitTaskfunction is commented out. Keeping large blocks of commented code can clutter the codebase.
- If the test is no longer needed, consider removing it.
- If it's a work in progress, add a
TODOcomment explaining why it's commented out and when it will be revisited.precompiles/avs/tx.go (2)
193-195: Consider renaming 'EmitOperatorOuted' for clarityThe function
EmitOperatorOutedmay not clearly convey its purpose. To improve readability and maintain consistency with other event functions, consider renaming it toEmitOperatorUnboundorEmitOperatorDeregistered.
193-195: Potential inconsistency in event function namingThe event emission function
EmitOperatorOuteduses the term "Outed," which may be unclear. For consistency and clarity, consider using a term likeEmitOperatorUnregisteredorEmitOperatorRemoved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- precompiles/avs/IAVSManager.sol (4 hunks)
- precompiles/avs/abi.json (7 hunks)
- precompiles/avs/avs.go (3 hunks)
- precompiles/avs/avs_test.go (7 hunks)
- precompiles/avs/events.go (2 hunks)
- precompiles/avs/query.go (4 hunks)
- precompiles/avs/query_test.go (3 hunks)
- precompiles/avs/tx.go (11 hunks)
- x/avs/keeper/avs_test.go (2 hunks)
- x/avs/keeper/keeper.go (4 hunks)
- x/avs/keeper/params.go (1 hunks)
- x/avs/keeper/task.go (11 hunks)
- x/avs/types/expected_keepers.go (3 hunks)
🧰 Additional context used
📓 Learnings (1)
x/avs/types/expected_keepers.go (1)
Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore#176 File: x/appchain/coordinator/types/expected_keepers.go:35-37 Timestamp: 2024-10-13T15:19:37.775Z Learning: The `OperatorKeeper` interface in `x/appchain/coordinator/types/expected_keepers.go` is not controlled by the project and cannot be modified.
🔇 Additional comments (52)
x/avs/types/expected_keepers.go (1)
4-5: LGTM: Import statement forcontextpackage.The addition of the
contextimport is appropriate and necessary for the new method signatures that usecontext.Contextas a parameter.x/avs/keeper/params.go (1)
105-114: LGTM: TaskResultParams struct is well-defined.The
TaskResultParamsstruct is correctly implemented with appropriate fields and types for managing task results. The use ofcommon.AddressforTaskContractAddressis consistent with Ethereum-related functionality.x/avs/keeper/avs_test.go (2)
Line range hint
141-191: Clarify changes to TestUpdateAVSInfoWithOperator_RegisterThe AI-generated summary mentions updates to error handling for cases where an AVS is already registered and modifications to the registration process. However, these changes are not visible in the provided diff for this function.
Could you please clarify:
- What specific changes were made to this function?
- How do these changes align with the PR objectives of "Adding Missing Events" and "Adding Missing Pre-compiled Interfaces"?
To help identify the changes, please run the following script:
#!/bin/bash # Show the git diff for this specific function git diff origin/main -- x/avs/keeper/avs_test.go | sed -n '/^func (suite \*AVSTestSuite) TestUpdateAVSInfoWithOperator_Register()/,/^}/p'
Line range hint
1-191: Alignment of changes with PR objectivesThe changes visible in this file primarily focus on adjusting test cases, particularly in
TestUpdateAVSInfo_DeRegister. However, the stated PR objectives mention:
- Adding Missing Events
- Adding Missing Pre-compiled Interfaces
- Fixing Audit TOW-02
The changes in this file don't directly address these objectives. Could you please clarify:
- Are the changes addressing these objectives located in other files not provided for this review?
- If so, could you provide those files for review to ensure a comprehensive assessment of the PR?
- If the changes in this file are indirectly related to the objectives, could you explain the connection?
To help identify relevant changes across the codebase, please run:
#!/bin/bash # Search for new event additions git diff origin/main | grep '+ *Event' # Search for new interface additions git diff origin/main | grep '+ *interface' # Search for changes related to audit finding TOW-02 git diff origin/main | grep -i 'TOW-02'x/avs/keeper/task.go (9)
28-28: LGTM: Consistent case-insensitive address handlingThe modification to use
strings.ToLower()ontask.TaskContractAddressensures consistent, case-insensitive handling of Ethereum addresses. This change aligns with best practices and improves the robustness of the system.
39-39: LGTM: Consistent case-insensitive address handling in retrievalThe use of
strings.ToLower()ontaskContractAddressensures consistent, case-insensitive retrieval of task information. This change aligns with the modification inSetTaskInfo, maintaining consistency throughout the system.
53-53: LGTM: Consistent case-insensitive task existence checkThe addition of
strings.ToLower()ontaskContractAddressensures consistent, case-insensitive checking of task existence. This change maintains consistency with the modifications inSetTaskInfoandGetTaskInfo.
217-219: LGTM: Consistent case-insensitive handling of operator and task addressesThe modification to use
strings.ToLower()on bothinfo.OperatorAddressandinfo.TaskContractAddressensures consistent, case-insensitive handling of operator and task contract addresses. This change aligns with the modifications in other functions and improves the overall consistency of address handling in the system.
278-278: LGTM: Consistent case-insensitive handling in second phaseThe use of
strings.ToLower()on bothinfo.OperatorAddressandinfo.TaskContractAddressin this part of the function ensures consistent, case-insensitive handling of addresses in the second phase of task result submission. This change maintains consistency with the first phase and other functions.
293-294: LGTM: Consistent case-insensitive task result existence checkThe addition of
strings.ToLower()on bothoperatorAddressandtaskContractAddressensures consistent, case-insensitive checking of task result existence. This change maintains consistency with the modifications in other functions and improves the robustness of the system.
304-305: LGTM: Consistent case-insensitive task result retrievalThe use of
strings.ToLower()on bothoperatorAddressandtaskContractAddressensures consistent, case-insensitive retrieval of task result information. This change aligns with the modifications in other functions and maintains the overall consistency of address handling in the system.
363-364: LGTM: Consistent case-insensitive handling of challenged task informationThe addition of
strings.ToLower()on bothoperatorAddressandtaskAddr.String()ensures consistent, case-insensitive handling of operator and task addresses for challenged information. This change maintains consistency with the modifications in other functions and improves the overall robustness of the system.
377-378: LGTM: Consistent case-insensitive check for challenged task informationThe use of
strings.ToLower()on bothoperatorAddressandtaskContractAddressensures consistent, case-insensitive checking of challenged task information existence. This change aligns with the modifications in other functions and maintains the overall consistency of address handling in the system.precompiles/avs/abi.json (15)
5-25: LGTM: New eventAVSDeregisteredaddedThe new event
AVSDeregisteredhas been correctly implemented with appropriate parameters. The indexed sender address will allow for efficient filtering of these events.
27-50: LGTM: New eventAVSRegisteredaddedThe new event
AVSRegisteredhas been correctly implemented, maintaining consistency with theAVSDeregisteredevent structure. This consistency in event design is commendable.
52-75: LGTM: New eventAVSUpdatedaddedThe new event
AVSUpdatedhas been correctly implemented, maintaining consistency with the other AVS-related events. This consistent approach to event design enhances the overall quality of the contract's event system.
120-137: LGTM: New eventOperatorJoinedaddedThe new event
OperatorJoinedhas been correctly implemented with appropriate parameters. The structure is consistent with other events in the contract, maintaining a coherent event system.
139-156: LGTM: New eventOperatorOutedaddedThe new event
OperatorOutedhas been correctly implemented, maintaining consistency with theOperatorJoinedevent structure. This consistent approach to event design enhances the overall quality and usability of the contract's event system.
158-181: LGTM: New eventOperatorRegisteredToExocoreaddedThe new event
OperatorRegisteredToExocorehas been correctly implemented. The inclusion of ametaInfoparameter adds valuable context to the registration process, which could be beneficial for logging and debugging purposes.
183-206: LGTM: New eventPublicKeyRegisteredaddedThe new event
PublicKeyRegisteredhas been correctly implemented. This event will be useful for tracking public key registrations, which is crucial for maintaining the security and integrity of the system.
454-471: LGTM: New functiongetAVSInfoaddedThe new
getAVSInfofunction has been correctly implemented as aviewfunction. It provides a way to retrieve theepochIdentifierfor a given AVS address, which could be useful for synchronization or validation purposes in an epoch-based system.
532-532: Approved:getOptInOperatorschanged toviewThe
getOptInOperatorsfunction has been correctly updated to haveviewstate mutability. This change improves gas efficiency and accurately reflects that the function doesn't modify state.
776-786: Approved:submitProofrenamed toregisterOperatorToExocorewith updated parametersThe function has been renamed from
submitProoftoregisterOperatorToExocore, which better reflects its purpose. The parameters have been updated to includesenderandmetaInfo, aligning with the newOperatorRegisteredToExocoreevent. These changes improve the clarity and functionality of the operator registration process.Please run the following script to check for any code that might need updating due to this function change:
#!/bin/bash # Search for calls to the old submitProof function rg --type solidity "submitProof\s*\(" -A 2
269-316: LGTM: New eventTaskSubmittedByOperatoradded, but note its complexityThe new event
TaskSubmittedByOperatorhas been correctly implemented. It captures detailed information about task submissions, including cryptographic proof (BLS signature) and stage information. While comprehensive, be mindful of the event's complexity and potential gas costs due to its large payload.To ensure the BLS signature is being used correctly, please run:
#!/bin/bash # Check for proper BLS signature usage rg --type solidity "blsSignature" -A 5 | ast-grep --lang solidity --pattern 'function $_($$) { $$$ $blsSignature $$$ }'
77-118: Approved:ChallengeInitiatedevent updated, but verify impactThe
ChallengeInitiatedevent has been updated with improved naming consistency (taskID) and a change fromtaskContractAddresstooperatorAddress. While these changes enhance the event's clarity, they may impact existing code that relies on the previous event structure.Please run the following script to check for any code that might be affected by this change:
✅ Verification successful
Verified:
ChallengeInitiatedevent changes have no adverse impactsThe update to the
ChallengeInitiatedevent, including renamingtaskIdtotaskIDand changingtaskContractAddresstooperatorAddress, has been verified. No references to the old parameters were found in the codebase, ensuring that existing code remains unaffected by these changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of the old event structure rg --type solidity "ChallengeInitiated.*taskId|ChallengeInitiated.*taskContractAddress"Length of output: 122
Script:
#!/bin/bash # Search for usages of the old event structure in Solidity files rg "ChallengeInitiated.*taskId|ChallengeInitiated.*taskContractAddress" --glob "*.sol"Length of output: 88
Line range hint
208-268: Approved:TaskCreatedevent expanded, but consider gas costsThe
TaskCreatedevent has been significantly expanded with additional parameters, providing more comprehensive information about created tasks. While this enhances the event's utility, be aware that the increased payload size may impact gas costs for emitting this event.Please run the following script to estimate the gas cost increase:
551-552: Approved:getRegisteredPubkeychanged toview, verify implementationThe
getRegisteredPubkeyfunction has been updated frompuretoviewstate mutability. This change suggests that the function now reads state variables, likely to access stored public keys.Please run the following script to verify the function's implementation:
Line range hint
397-407: Approved:createTasknow returnstaskIDThe
createTaskfunction has been improved to return thetaskIDof the created task. This is a good practice that enhances the function's usability. However, ensure that all existing calls to this function are updated to handle the new return value.Please run the following script to check for any code that might need updating:
✅ Verification successful
Action Required: Adjust Shell Script to Properly Search Solidity Files
Please run the following corrected script to locate all calls to
createTaskwithin Solidity (.sol) files:
Action Required: Use
fdto Locate Solidity Files and Search forcreateTaskCallsPlease run the following script to find all instances of
createTask(within Solidity (.sol) files:
Verification Successful: No Updates Required for
createTaskCallsThe
createTaskfunction inprecompiles/avs/IAVSManager.solhas been updated to returntaskID. Upon verification, no existing calls to this function were found in the codebase that require handling the new return value.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to createTask that might need updating rg --type solidity "createTask\s*\(" -A 2Length of output: 76
Script:
# Search for calls to createTask that might need updating in .sol files rg "createTask\s*\(" --include '*.sol' -A 2Length of output: 487
Script:
# Find all .sol files and search for 'createTask(' within them fd -e sol | xargs rg "createTask\s*\("Length of output: 97
precompiles/avs/query.go (7)
5-6: Imports added are necessary and correctly usedThe addition of
"strconv"and"strings"packages is appropriate as they are required for string conversion and error handling in the new methods.
9-9: Importingavstypeis requiredThe import of
avstypeis necessary for accessing AVS type definitions used in the new methods.
23-25: New method constants follow naming conventionsThe constants
MethodGetAVSInfo,MethodGetTaskInfo, andMethodIsOperatorare properly defined and consistent with existing method naming conventions.
64-64: Updated method call inGetOptedInOperatorAccAddrsThe method call is updated to
GetOptedInOperatorListByAVS, which reflects the new logic for retrieving opted-in operators by AVS. Ensure that this method exists and returns data in the expected format compatible with this function.
117-142: Check return value when AVS does not existWhen the AVS does not exist, the method returns an empty string. Ensure that this behavior aligns with the expected output defined in the ABI and that the caller can handle an empty string appropriately.
144-165: Ensure consistency of operator address type inIsOperatorThe operator address is extracted as a
stringfromargs[0]. Ensure that the ABI definition forMethodIsOperatorspecifies astringtype for the operator address to match this usage.
195-196: Ensure output matches expected ABI return typesThe method packs an array of
uint64as the return value. Verify that this matches the output parameters defined in the ABI forGetTaskInfoand that the caller can correctly interpret the returned data.precompiles/avs/avs.go (2)
161-174: New methods correctly implemented with consistent error handlingThe methods
MethodRegisterOperatorToExocoreandMethodOperatorSubmitTaskhave been properly added to theRunfunction with consistent error handling. The error logging and default output packing align with the existing implementation.
197-198: Methods correctly classified as transactionsThe methods
MethodRegisterOperatorToExocoreandMethodOperatorSubmitTaskhave been appropriately added to theIsTransactionmethod, ensuring they are recognized as transactions.precompiles/avs/IAVSManager.sol (4)
16-30: Approved: Addition of New EventsThe newly added events enhance the interface's capability to emit important state changes and actions related to AVS management, operator activities, task creation, and submissions. The event definitions are correctly structured with appropriate parameters and align with Solidity's best practices.
180-186: Approved: Addition ofregisterOperatorToExocoreFunctionThe new
registerOperatorToExocorefunction allows operators to register with metadata. The function signature and parameters are appropriate and adhere to Solidity conventions.
208-208: Correct Use ofviewModifier in Getter FunctionsThe functions
getRegisteredPubkeyandgetOptInOperatorshave been updated frompuretoview, which is appropriate since they read state variables. This change accurately reflects their behavior and complies with Solidity's function visibility requirements.Also applies to: 212-212
244-249: Approved: Addition ofisOperatorFunctionThe
isOperatorfunction correctly checks whether a given operator address is registered on Exocore. The function signature is appropriate, and using aviewfunction aligns with Solidity best practices.x/avs/keeper/keeper.go (4)
136-138: Confirm the unbonding period condition logicIn the
DeRegisterActioncase ofUpdateAVSInfo, the condition checks if the AVS has been active for less than or equal to the unbonding period:if epoch.CurrentEpoch - int64(avsInfo.GetInfo().StartingEpoch) <= int64(avsInfo.Info.AvsUnbondingPeriod) { return errorsmod.Wrap(types.ErrUnbondingPeriod, fmt.Sprintf("not qualified to deregister %s", avsInfo)) }Please verify that this logic correctly enforces that an AVS cannot be deregistered until after the unbonding period has passed.
223-223: Update callers to handle new return value ofCreateAVSTaskThe
CreateAVSTaskmethod's signature has changed to return auint64task ID and anerror:func (k Keeper) CreateAVSTask(ctx sdk.Context, params *TaskInfoParams) (uint64, error)Ensure that all callers of this function are updated to handle the new task ID and any potential errors appropriately.
226-248: Consistent error handling inCreateAVSTaskWithin
CreateAVSTask, errors are now returned along with a zero task ID:return 0, errorsmod.Wrap(...)Confirm that this pattern is consistent throughout the method and that the zero task ID is appropriately handled by callers in error cases to prevent unintended behavior.
248-248: Handle potential error when retrieving opt-in operator listIn
CreateAVSTask, when retrieving the opt-in operator list:operatorList, err := k.operatorKeeper.GetOptedInOperatorListByAVS(ctx, avsInfo.AvsAddress) if err != nil { return 0, errorsmod.Wrap(err, "CreateAVSTask: failed to get opt-in operators") }Ensure proper error handling and consider logging the error for troubleshooting purposes.
precompiles/avs/query_test.go (1)
325-325: Ensure Consistent Operator Address UsageThe operator address is hard-coded as:
operatorAddr := "exo13h6xg79g82e2g2vhjwg7j4r2z2hlncelwutkjr"Verify that this address matches the one set in
suite.prepareOperator()to prevent inconsistencies. Ifsuite.prepareOperator()initializes a different operator address, consider using a variable to maintain consistency.precompiles/avs/avs_test.go (1)
69-83: New methods correctly included in transaction testsThe methods
avs.MethodChallenge,avs.MethodRegisterOperatorToExocore, andavs.MethodOperatorSubmitTaskare appropriately added to thetestCasesinTestIsTransaction()for transaction verification.precompiles/avs/tx.go (5)
62-64: Ensure event emissions handle errors consistentlyAcross various functions, after emitting events, errors are checked and returned if event emission fails. Verify that all event emission functions properly handle errors and that any necessary cleanup or rollback occurs in case of failures to maintain consistency and reliability.
Also applies to: 99-101, 134-136, 164-166, 217-220, 276-278, 329-331, 367-369, 429-431
Line range hint
287-333: Review 'RegisterBLSPublicKey' function for completenessThe
RegisterBLSPublicKeyfunction appears to handle input parameters and error checking appropriately. Ensure that all BLS public key registration logic aligns with the expected cryptographic standards and that input validations are thorough to prevent potential security issues.
335-371: Function 'RegisterOperatorToExocore' implemented correctlyThe
RegisterOperatorToExocorefunction effectively registers an operator with the provided metadata. Input validations and error handling are properly addressed.
373-431: Function 'OperatorSubmitTask' appears well-structuredThe
OperatorSubmitTaskfunction includes comprehensive parameter checks and assigns values correctly. Ensure thatSubmitTaskResultin theavsKeepercorrectly processes the task result and that the emitted eventEmitTaskSubmittedByOperatoraccurately reflects the operation.
174-176:⚠️ Potential issueIncorrect method reference in argument validation
In the
UnbindOperatorToAVSfunction, the argument length check usesMethodRegisterOperatorToAVSinstead ofMethodDeregisterOperatorFromAVS. This could result in incorrect validation if the methods have different input parameters.Apply this diff to fix the issue:
- if len(args) != len(p.ABI.Methods[MethodRegisterOperatorToAVS].Inputs) { + if len(args) != len(p.ABI.Methods[MethodDeregisterOperatorFromAVS].Inputs) {Likely invalid or redundant comment.
MaxMustermann2
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (10)
precompiles/avs/IAVSManager.sol (2)
Line range hint
135-149: LGTM: Improved return value for createTaskThe updated
createTaskfunction now returns both asuccessboolean and ataskID. This is a good improvement as it provides more information to the caller.Minor suggestion: Consider using a struct for the return values to improve readability and maintainability, especially if more return values might be added in the future.
Example of using a struct for return values:
struct CreateTaskResult { bool success; uint64 taskID; } function createTask( // ... existing parameters ... ) external returns (CreateTaskResult memory result);This approach makes it easier to add or modify return values in the future without changing the function signature.
Line range hint
205-254: LGTM: Enhanced query functionalityThe addition of new query functions (
getAVSInfo,getTaskInfo,isOperator,getCurrentEpoch) and the update of existing functions toviewsignificantly improve the interface's ability to retrieve system information.Minor suggestion for consistency:
- Consider using
string memoryinstead ofstringfor function parameters across all query functions. This explicitly indicates that the string is stored in memory and is consistent with other functions in the interface.Example of consistent parameter declaration:
function isOperator( string memory operatorAddr ) external view returns (bool); function getCurrentEpoch( string memory epochIdentifier ) external view returns (int64 currentEpoch);x/avs/keeper/keeper.go (2)
Line range hint
228-269: LGTM: Improved CreateAVSTask function with task ID returnThe changes to
CreateAVSTaskare well-implemented:
- The function now returns the task ID along with any error, which is more informative for callers.
- Error handling has been updated consistently throughout the function.
- New error checking for opt-in operators adds robustness.
- Task ID is now set using
k.GetTaskID, ensuring consistent ID generation.These changes improve the function's usability and error handling.
Consider using a named return value for the task ID to make the code more self-documenting. For example:
func (k Keeper) CreateAVSTask(ctx sdk.Context, params *TaskInfoParams) (taskID uint64, err error) { // ... existing code ... return task.TaskId, nil }This makes it clear what the uint64 return value represents and allows you to simply use
return 0, errin error cases.
449-459: LGTM: New SubmitTaskResult functionThe new
SubmitTaskResultfunction provides a clean interface for submitting task results. It correctly encapsulates the creation ofTaskResultInfoand the call toSetTaskResultInfo.Consider adding some basic input validation to ensure the integrity of the submitted data. For example:
func (k Keeper) SubmitTaskResult(ctx sdk.Context, params *TaskResultParams) error { if params == nil { return errors.New("params cannot be nil") } if params.TaskID == 0 { return errors.New("TaskID must be non-zero") } if params.OperatorAddress == "" { return errors.New("OperatorAddress cannot be empty") } // ... rest of the function ... }This would help catch potential issues earlier and provide more meaningful error messages.
precompiles/avs/query_test.go (6)
322-378: LGTM! Consider adding error test cases.The implementation of
TestGetRegisteredPubkeylooks good and covers the successful retrieval of an existing public key. However, to improve test coverage, consider adding test cases for error scenarios such as:
- Attempting to retrieve a public key for a non-existent operator.
- Passing an invalid operator address.
These additional test cases will ensure the function handles error conditions correctly.
380-447: LGTM! Expand test coverage for AVS info fields.The implementation of
TestGetAVSInfois good, but it currently only checks theEpochIdentifierfield of the returned AVS info. To ensure comprehensive testing, consider expanding the test to verify other important fields of the AVS info, such as:
- Name
- AvsAddress
- SlashAddr
- AvsOwnerAddress
- AssetIDs
- AvsUnbondingPeriod
- MinSelfDelegation
- StartingEpoch
- MinOptInOperators
- MinTotalStakeAmount
- AvsSlash
- AvsReward
- TaskAddr
This will provide a more thorough validation of the
GetAVSInfomethod.
449-491: LGTM! Add test case for non-existent operator.The implementation of
TestIsoperatorcorrectly tests the positive case where an operator exists. To improve test coverage, consider adding a test case for a non-existent operator. This will ensure that theIsOperatormethod correctly returnsfalsefor operators that are not registered.Example additional test case:
{ "success - non-existent operator", func() []interface{} { return []interface{}{ "exo1nonexistentoperator", } }, func(bz []byte) { var out bool err := suite.precompile.UnpackIntoInterface(&out, avsManagerPrecompile.MethodIsOperator, bz) suite.Require().NoError(err, "failed to unpack output", err) suite.Require().Equal(false, out) }, 100000, false, "", },This addition will ensure that the method correctly handles both existing and non-existing operators.
492-550: LGTM! Expand test coverage for task info fields.The implementation of
TestGetTaskInfois good, but it currently only checks a subset of the task info fields (StartingEpoch, TaskResponsePeriod, and TaskStatisticalPeriod). To ensure comprehensive testing, consider expanding the test to verify all relevant fields of the task info, such as:
- TaskContractAddress
- Name
- TaskId
- Hash
- TaskTotalPower
This will provide a more thorough validation of the
GetTaskInfomethod and ensure that all fields are correctly retrieved and unpacked.
551-591: LGTM! Enhance test robustness and coverage.The implementation of
TestGetCurrentEpochis good for a basic test case. However, to make it more robust and comprehensive, consider the following improvements:
- Add a setUp function to explicitly set the current epoch for the test, ensuring the expected value is correct.
- Test multiple epoch identifiers (e.g., DayEpochID, WeekEpochID) to ensure the method works correctly for different types of epochs.
- Add a test case for an invalid or non-existent epoch identifier to check error handling.
Example improvements:
setUp := func(epochID string, epochNumber int64) { // Set up the epoch with a known value err := suite.App.EpochsKeeper.SetEpochInfo(suite.Ctx, epochstypes.EpochInfo{ Identifier: epochID, CurrentEpoch: epochNumber, // ... other necessary fields }) suite.Require().NoError(err) } testCases := []avsTestCases{ { "success - day epoch", func() []interface{} { setUp(epochstypes.DayEpochID, 5) return []interface{}{ epochstypes.DayEpochID, } }, func(bz []byte) { var out int64 err := suite.precompile.UnpackIntoInterface(&out, avsManagerPrecompile.MethodGetCurrentEpoch, bz) suite.Require().NoError(err, "failed to unpack output", err) suite.Require().Equal(int64(5), out) }, 100000, false, "", }, { "success - week epoch", func() []interface{} { setUp(epochstypes.WeekEpochID, 2) return []interface{}{ epochstypes.WeekEpochID, } }, func(bz []byte) { var out int64 err := suite.precompile.UnpackIntoInterface(&out, avsManagerPrecompile.MethodGetCurrentEpoch, bz) suite.Require().NoError(err, "failed to unpack output", err) suite.Require().Equal(int64(2), out) }, 100000, false, "", }, { "fail - invalid epoch identifier", func() []interface{} { return []interface{}{ "InvalidEpochID", } }, func(bz []byte) {}, 100000, true, "epoch not found", }, }These improvements will make the test more robust and ensure it covers various scenarios and error cases.
Line range hint
1-591: Overall good test coverage with room for improvement.The test suite for the AVS manager precompile methods is well-structured and covers the basic functionality of each method. The consistent use of the
avsTestCasesstruct and thesetUpfunctions for each test provides a clear and maintainable approach to testing.To further enhance the quality and robustness of the test suite, consider the following general improvements:
- Increase error case coverage: Add more test cases that check for error conditions, such as invalid inputs or edge cases.
- Enhance field verification: When testing methods that return complex structures (like AVS info or task info), ensure all relevant fields are verified, not just a subset.
- Use table-driven tests: Consider using table-driven tests for methods that require multiple similar test cases with different inputs.
- Add negative test cases: Include tests that verify the behavior when operations are expected to fail (e.g., querying non-existent entities).
- Test boundary conditions: Add tests for boundary conditions, such as minimum and maximum values for numeric fields.
- Improve setup and teardown: Ensure proper setup and teardown for each test to avoid potential state leakage between tests.
Implementing these suggestions will result in a more comprehensive and robust test suite, increasing confidence in the correctness of the AVS manager precompile implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- precompiles/avs/IAVSManager.sol (4 hunks)
- precompiles/avs/abi.json (8 hunks)
- precompiles/avs/avs.go (3 hunks)
- precompiles/avs/query.go (6 hunks)
- precompiles/avs/query_test.go (3 hunks)
- x/avs/keeper/keeper.go (5 hunks)
🧰 Additional context used
🔇 Additional comments (21)
precompiles/avs/query.go (4)
5-6: LGTM: New imports and constants addedThe new imports (
strconvandavstype) and constants for method names are appropriate for the newly added functionality.Also applies to: 9-9, 23-26
65-65: LGTM: Method call updated in GetOptedInOperatorAccAddrsThe change from
GetOptInOperatorstoGetOptedInOperatorListByAVSappears to be a refactoring to use a more specific method name, which likely better reflects its functionality.
145-166: LGTM: IsOperator method implemented correctlyThe
IsOperatormethod is implemented correctly. It properly validates the input, converts the address to the required format, and uses the appropriate keeper method to check if the address is an operator.
Line range hint
1-219: Overall assessment: Good additions with minor improvements neededThe changes to
precompiles/avs/query.gosignificantly enhance the AVS functionality by adding new query methods (GetAVSInfo,IsOperator,GetTaskInfo,GetCurrentEpoch) and refining existing ones. The implementations are generally correct, but there are a few areas where improvements can be made:
- Error handling in
GetAVSInfoandGetTaskInfocan be enhanced usingerrors.Is.- Type assertion for
taskIDinGetTaskInfocan be improved to handle*big.Int.These minor adjustments will further improve the robustness and consistency of the code.
precompiles/avs/IAVSManager.sol (4)
16-29: LGTM: Comprehensive event system addedThe new events provide excellent visibility into the AVS system's operations. They cover all major actions and follow consistent naming and parameter conventions. The use of
indexedfor sender addresses is a good practice for efficient event filtering.
180-187: LGTM: New function for operator registrationThe
registerOperatorToExocorefunction is a valuable addition to the interface, providing a way to register operators to the Exocore system. The function signature is clear and concise, with appropriate parameter types.
Line range hint
1-254: Overall: Significant and positive enhancements to the IAVSManager interfaceThe changes to the
IAVSManagerinterface represent a substantial improvement in functionality and completeness for the AVS system. Key improvements include:
- Comprehensive event system for tracking important actions.
- New
TaskInfostruct for detailed task representation.- Enhanced function return values (e.g.,
createTask).- New functions for operator registration and task submission.
- Expanded query capabilities for system information retrieval.
These changes provide a more robust and feature-rich interface for AVS management. While there are a few minor optimizations suggested in previous comments, the overall direction and implementation of these changes are commendable.
To further improve the interface:
- Consider implementing the suggested optimizations for the
TaskInfostruct andstageparameter.- Ensure consistency in parameter declarations across all functions.
- Review and update documentation to reflect all new functionality.
188-203: 🛠️ Refactor suggestionUse an enum for the
stageparameterThe
operatorSubmitTaskfunction is a crucial addition for task result submission. However, as previously suggested in a past review comment, using an enum for thestageparameter would improve type safety and readability.Implement the enum as suggested in the previous review:
enum SubmissionStage { Stage1, Stage2 } function operatorSubmitTask( address sender, uint64 taskID, bytes calldata taskResponse, bytes calldata blsSignature, address taskContractAddress, SubmissionStage stage ) external returns (bool success);This change ensures that only valid stages can be passed to the function and improves code clarity.
precompiles/avs/abi.json (7)
Line range hint
5-317: New events look good and provide comprehensive logging.The newly added events (AVSDeregistered, AVSRegistered, AVSUpdated, ChallengeInitiated, OperatorJoined, OperatorOuted, OperatorRegisteredToExocore, PublicKeyRegistered, TaskCreated, TaskSubmittedByOperator) are well-structured and provide detailed logging for various actions in the AVS system. They follow a consistent format and use appropriate indexing for efficient event filtering.
401-407: Improved createTask function with taskID return value.The modification to the createTask function to return both a success boolean and a taskID (uint64) is a good improvement. This allows the caller to immediately know the ID of the created task, which can be useful for further operations or tracking.
Line range hint
551-571: Correct stateMutability changes for getter functions.The stateMutability of getOptInOperators and getRegisteredPubkey functions has been appropriately changed to "view". This is correct as these functions don't modify the contract state. The change improves gas efficiency and allows these functions to be called without creating a transaction.
454-472: New getAVSInfo function looks good.The newly added getAVSInfo function is correctly implemented as a view function. It provides a way to retrieve the epoch identifier for a given AVS address, which can be useful for various operations in the system.
492-510: New getCurrentEpoch function is well-implemented.The newly added getCurrentEpoch function is correctly implemented as a view function. It provides a way to get the current epoch for a given epoch identifier, which is essential for time-based operations in the system.
573-596: New getTaskInfo function is properly implemented.The newly added getTaskInfo function is correctly implemented as a view function. It provides a way to retrieve information about a specific task using the task address and ID, returning an array of uint64 values. This can be useful for various operations and queries in the system.
Line range hint
795-814: Improved clarity and functionality in registerOperatorToExocore.The function has been appropriately renamed from submitProof to registerOperatorToExocore, providing better clarity about its purpose. The addition of the metaInfo parameter allows for more detailed information to be provided during operator registration. These changes enhance the functionality and usability of the operator registration process.
x/avs/keeper/keeper.go (3)
65-69: LGTM: New getter function for epochsKeeperThe addition of
GetEpochKeeperfunction is appropriate and consistent with the existing codebase structure. It provides necessary access to theepochsKeeperfor other parts of the system.
141-141: Verify the change in unbonding period check logicThe condition for checking the unbonding period has been inverted. This change allows deregistration if
(current_epoch - starting_epoch) <= unbonding_period, which seems to contradict the typical unbonding period concept. Please clarify if this is intentional and explain the reasoning behind this change.Could you provide more context on why this change was made? It appears to allow earlier deregistration than the original logic intended.
Line range hint
301-321: Verify the refactoring of opt-in operator retrievalThe
GetOptInOperatorsfunction has been removed, and its functionality has been moved to theoperatorKeeper. This refactoring improves the separation of concerns. However, it's important to ensure that the new implementation inoperatorKeepermaintains the same functionality and performance characteristics.Please confirm that the
operatorKeeper.OptInandoperatorKeeper.OptOutmethods have been updated to handle the functionality previously managed byGetOptInOperators. Additionally, verify that any other parts of the codebase that may have usedGetOptInOperatorshave been updated accordingly.✅ Verification successful
Verified:
GetOptInOperatorsreferences successfully removedNo active references to
GetOptInOperatorsremain in the codebase, ensuring the refactoring is complete. Please ensure that the generated filex/avs/types/tx.pb.gois up-to-date and correctly synchronized with the current code changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to GetOptInOperators rg "GetOptInOperators" --type goLength of output: 103
precompiles/avs/avs.go (3)
184-189: Confirm default return value in error handling forMethodRegisterOperatorToExocoreIn the error handling for
MethodRegisterOperatorToExocore, you're packingfalseas the default value:bz, err = method.Outputs.Pack(false)If this method returns a boolean indicating success or failure,
falseis an appropriate default value. Please confirm that this aligns with the method's expected output.
191-196: Confirm default return value in error handling forMethodOperatorSubmitTaskFor
MethodOperatorSubmitTask, the error handling packsfalseas the default return value:bz, err = method.Outputs.Pack(false)If the method's output is a boolean indicating the success of the operation,
falseis suitable. Ensure that this matches the expected return type.
219-223: New methods correctly categorized inIsTransactionThe methods
MethodRegisterOperatorToExocoreandMethodOperatorSubmitTaskhave been appropriately added to the transactions list, andMethodGetAVSInfo,MethodGetTaskInfo,MethodIsOperator, andMethodGetCurrentEpochare correctly identified as queries.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
x/operator/keeper/opt.go (1)
87-99: Consider enhancing error handling consistency.
The implementation looks good with proper context caching and error logging. However, we could improve error handling consistency.
Consider wrapping the errors with additional context for better debugging:
if err := k.OptIn(ctx, operatorAddress, avsAddr); err != nil {
- logger.Error("OptInWithConsKey", "error", err)
+ err = errorsmod.Wrapf(err, "failed to opt in operator %s for AVS %s", operatorAddress, avsAddr)
+ logger.Error("OptInWithConsKey", "error", err)
return err
}
chainID, _ := k.avsKeeper.GetChainIDByAVSAddr(ctx, avsAddr)
if err := k.SetOperatorConsKeyForChainID(ctx, operatorAddress, chainID, key); err != nil {
- logger.Error("OptInWithConsKey", "error", err)
+ err = errorsmod.Wrapf(err, "failed to set operator cons key for chain %s", chainID)
+ logger.Error("OptInWithConsKey", "error", err)
return err
}x/dogfood/keeper/abci.go (2)
83-87: Enhance error logging for consensus key deserialization.
Since this error "should never happen", consider adding more context to the error log such as the validator address or key details to aid in debugging if this occurs in production.
logger.Error(
"error deserializing consensus public key",
+ "validator", validator.String(),
"error", err,
)Line range hint 105-169: LGTM! Consider defining capacity multiplier as a constant.
The logging improvements and power update logic look good. The capacity pre-allocation with maxVals*2 is well-commented, but consider defining this multiplier as a named constant for better maintainability.
+const validatorUpdateCapacityMultiplier = 2
-res := make([]keytypes.WrappedConsKeyWithPower, 0, maxVals*2)
+res := make([]keytypes.WrappedConsKeyWithPower, 0, maxVals*validatorUpdateCapacityMultiplier)x/dogfood/keeper/opt_out_test.go (1)
118-118: Consider enhancing the comment with specific requirement details.
While the comment clarifies that self-delegation is required, it could be more specific about the actual requirement threshold.
Consider updating the comment to include the specific threshold:
- // opt in successfully after the self delegation meet the requirement
+ // opt in successfully after the self delegation meets the minimum requirement (totalAmountInUSD)📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- precompiles/avs/utils_test.go (1 hunks)
- x/avs/keeper/avs.go (4 hunks)
- x/dogfood/keeper/abci.go (8 hunks)
- x/dogfood/keeper/opt_out_test.go (2 hunks)
- x/operator/keeper/impl_epoch_hook.go (2 hunks)
- x/operator/keeper/opt.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- precompiles/avs/utils_test.go
- x/avs/keeper/avs.go
🔇 Additional comments (7)
x/operator/keeper/impl_epoch_hook.go (2)
4-5: LGTM: Import addition is appropriate
The addition of the "strings" import is necessary for the new string manipulation functionality.
34-35: LGTM: Improved address case handling
The conversion of checksummed hex addresses to lowercase before updating voting power is a good practice for consistent address comparison.
Let's verify that address case handling is consistent across the codebase:
x/operator/keeper/opt.go (1)
85-85: LGTM! Clear parameter naming.
The parameter rename from ctx to originalCtx better reflects its purpose as the source context for caching.
x/dogfood/keeper/abci.go (3)
Line range hint 174-204: LGTM! Clear logging and proper power updates.
The final power calculations and validator set updates are well-structured with appropriate logging. The comment explaining why previous power doesn't need to be subtracted is particularly helpful for maintainability.
97-102: Review impact of returning empty validator updates on error.
Returning empty updates when failing to get vote power could affect consensus if this occurs during critical validator set changes. Consider if partial updates or previous state should be maintained in this error case.
#!/bin/bash
# Check for other instances where empty validator updates are returned
rg "return \[\]abci.ValidatorUpdate{}" 33-38: Consider enhancing error handling for undelegation operations.
While errors are logged, failing to decrement the undelegation hold count could lead to stuck undelegations. Consider adding metrics or alerts for monitoring these errors in production.
x/dogfood/keeper/opt_out_test.go (1)
32-35: LGTM: Good addition of explicit AVS existence check!
The added check improves test robustness by validating preconditions before attempting the opt-in operation.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
x/avs/types/utils.go (1)
74-78: Consider using iota for OperatorAction constants.
While the current implementation is correct, using iota would make it easier to maintain and extend the constants.
const (
- RegisterAction OperatorAction = 1
- DeRegisterAction OperatorAction = 2
- UpdateAction OperatorAction = 3
+ RegisterAction OperatorAction = iota + 1
+ DeRegisterAction
+ UpdateAction
)x/avs/keeper/avs.go (1)
109-111: Consider adding debug logging for skipped entries.
While the empty address check is good, consider adding debug logging when skipping entries to help with troubleshooting.
if avsInfo.AvsAddress == "" {
+ k.Logger(ctx).Debug("Skipping task result due to empty AVS address", "task_contract", info.TaskContractAddress)
return false
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/avs/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
- precompiles/avs/tx.go (11 hunks)
- proto/exocore/avs/v1/tx.proto (2 hunks)
- x/avs/client/cli/tx.go (5 hunks)
- x/avs/keeper/avs.go (6 hunks)
- x/avs/types/utils.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/avs/client/cli/tx.go
🧰 Additional context used
🪛 GitHub Check: break-check
proto/exocore/avs/v1/tx.proto
[failure] 239-239:
Field "7" with name "phase" on message "TaskResultInfo" changed option "json_name" from "stage" to "phase".
[failure] 239-239:
Field "7" on message "TaskResultInfo" changed type from "string" to "enum".
[failure] 239-239:
Field "7" on message "TaskResultInfo" changed name from "stage" to "phase".
🔇 Additional comments (9)
x/avs/types/utils.go (2)
10-16: LGTM! Well-structured enum type.
The implementation follows best practices:
- Uses uint8 for memory efficiency
- Starts from 1 to avoid zero-value ambiguity
- Clear and descriptive naming
100-107: LGTM! Well-implemented validation function.
The function properly validates CommitPhase values and provides clear error messages.
x/avs/keeper/avs.go (4)
7-7: LGTM: Import addition is appropriate.
The addition of the "strings" package is necessary for the new string manipulation functionality.
50-52: LGTM: Added proper zero address handling.
The addition of zero address check improves the robustness of the contract by explicitly handling uninitialized slash addresses.
78-78: LGTM: Standardized address case handling.
Converting addresses to lowercase ensures consistent address comparison throughout the system, following Ethereum's case-insensitive address handling convention.
91-91: LGTM: Enhanced input validation.
The improved condition properly handles both empty string and zero address cases, making the function more robust.
precompiles/avs/tx.go (3)
28-28: LGTM: Method constant follows naming convention.
The new constant MethodOperatorSubmitTask follows the established naming pattern.
58-60: LGTM: Consistent event emission pattern.
Event emission has been consistently added across all methods with proper error handling. The pattern follows best practices:
- Emit event before returning
- Return error if emission fails
- Use dedicated emit methods for each event type
Also applies to: 95-97, 130-132, 160-162, 189-191, 213-215, 272-274, 325-327
331-404: Verify task existence and operator authorization.
The function should verify:
- The task exists before accepting the submission
- The operator is authorized to submit results for this task
✅ Verification successful
Let me analyze the results and generate additional verification scripts to check for task existence and operator authorization checks.
The previous results show that SetTaskResultInfo in x/avs/keeper/task.go performs some validations, but let's verify the specific checks for task existence and operator authorization by looking at the implementation of these key methods.
Let me analyze the results and generate one final verification script to check the task existence validation.
From the previous results, we can see that operator authorization is properly checked in SetTaskResultInfo through:
k.operatorKeeper.IsOperator(ctx, opAccAddr)validation- Additional checks for frozen operators
We need to verify the task existence validation implementation.
Task existence and operator authorization are properly validated
The code already implements the necessary validations in SetTaskResultInfo:
- Task existence is verified through
GetTaskInfowhich returns an error if the task doesn't exist - Operator authorization is checked via
k.operatorKeeper.IsOperator(ctx, opAccAddr) - Additional operator checks include verifying if the operator is frozen
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are task existence and operator authorization checks elsewhere in the codebase
# Search for task existence validation
echo "Searching for task existence validation..."
rg -A 5 "SetTaskResultInfo|TaskResultInfo"
# Search for operator authorization checks
echo "Searching for operator authorization checks..."
rg -A 5 "OperatorAddress.*authorized|operator.*permission"
Length of output: 27074
Script:
#!/bin/bash
# Search for task existence validation in the keeper
rg -A 10 "GetTaskInfo.*ctx.*taskId"
# Search for operator validation in the keeper
rg -A 10 "IsOperator.*ctx.*operator"
Length of output: 6413
Script:
#!/bin/bash
# Search for GetTaskInfo implementation and task existence checks
rg -A 10 "func.*GetTaskInfo.*ctx.*task"
Length of output: 861
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
proto/exocore/avs/v1/tx.proto (1)
94-96: Enhance comment clarity for task periods.
The comments for task periods could be more descriptive. Consider adding details about:
- What constitutes a "challenge period"
- The units of measurement (blocks, time, etc.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/avs/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
- precompiles/avs/IAVSManager.sol (4 hunks)
- precompiles/avs/tx.go (11 hunks)
- proto/exocore/avs/v1/query.proto (2 hunks)
- proto/exocore/avs/v1/tx.proto (12 hunks)
- x/avs/client/cli/tx.go (5 hunks)
- x/avs/keeper/avs.go (6 hunks)
- x/avs/types/utils.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- proto/exocore/avs/v1/query.proto
🚧 Files skipped from review as they are similar to previous changes (2)
- x/avs/keeper/avs.go
- x/avs/types/utils.go
🧰 Additional context used
🪛 GitHub Check: break-check
proto/exocore/avs/v1/tx.proto
[failure] 230-230:
Field "7" with name "phase" on message "TaskResultInfo" changed option "json_name" from "stage" to "phase".
[failure] 230-230:
Field "7" on message "TaskResultInfo" changed type from "string" to "enum".
[failure] 230-230:
Field "7" on message "TaskResultInfo" changed name from "stage" to "phase".
🔇 Additional comments (15)
x/avs/client/cli/tx.go (5)
90-90: LGTM! Required flag addition is appropriate.
Making the phase flag required ensures that users explicitly specify the submission phase.
100-100: LGTM! Improved error handling in function signature.
The addition of error return improves error propagation and allows proper error handling at the caller level.
119-122: LGTM! Proper phase validation is implemented.
The validation ensures that only valid phase values are accepted, with proper type conversion.
131-131: LGTM! Message construction properly uses the new phase field.
The changes correctly implement the transition from stage to phase in the message structure.
Also applies to: 134-134
107-115: Consider adding length validation for decoded data.
While the hex decoding error handling is good, consider adding validation for the expected length of the decoded taskRes and sig bytes to ensure they meet the protocol requirements.
proto/exocore/avs/v1/tx.proto (3)
47-50: LGTM! Improved formatting.
The indentation changes enhance readability while maintaining protobuf style guidelines.
205-213: LGTM! Well-structured enum definition.
The Phase enum follows Protocol Buffer best practices with:
- Clear documentation for each value
- Proper UNSPECIFIED default value
- Logical progression of phases
251-263: LGTM! Improved RPC method formatting.
The removal of spaces between method names and parameters follows protobuf style guidelines.
precompiles/avs/IAVSManager.sol (4)
7-7: LGTM!
The constant declaration follows Solidity best practices and improves code organization.
Also applies to: 12-12
14-14: LGTM! Helpful comment about indexing behavior.
The comment clearly explains the hashing behavior of indexed parameters for different types.
219-221: LGTM! Well-documented isOperator function
The function is clearly documented and follows the interface's style.
20-30: 🛠️ Refactor suggestion
Consider splitting TaskCreated event parameters
Based on previous discussions, the event parameters could be logically grouped to improve readability and maintainability.
- event TaskCreated(
- string sender,
- uint64 taskId,
- address indexed taskContractAddress,
- string name,
- bytes hash,
- uint64 taskResponsePeriod,
- uint64 taskChallengePeriod,
- uint64 thresholdPercentage,
- uint64 taskStatisticalPeriod
- );
+ event TaskCreated(
+ string sender,
+ uint64 taskId,
+ address indexed taskContractAddress,
+ string name,
+ bytes hash
+ );
+ event TaskConfigSet(
+ uint64 taskId,
+ uint64 taskResponsePeriod,
+ uint64 taskChallengePeriod,
+ uint64 thresholdPercentage,
+ uint64 taskStatisticalPeriod
+ );Likely invalid or redundant comment.
precompiles/avs/tx.go (3)
28-28: LGTM: Method constant follows naming convention.
The new constant MethodOperatorSubmitTask follows the established naming pattern of other method constants in the file.
Line range hint 31-162: LGTM: Consistent stateDB parameter and event emission pattern.
The addition of the stateDB parameter and event emission calls across all methods follows a consistent pattern, improving the system's observability.
Also applies to: 165-216, 219-274, 277-329
389-397: 🛠️ Refactor suggestion
Consider adding timestamp to TaskResultInfo.
For audit and tracking purposes, it would be beneficial to include a timestamp when the task result was submitted.
Apply this diff to add timestamp tracking:
result := &avstypes.TaskResultInfo{
TaskId: resultParams.TaskID,
OperatorAddress: resultParams.CallerAddress.String(),
TaskContractAddress: resultParams.TaskContractAddress.String(),
TaskResponse: resultParams.TaskResponse,
BlsSignature: resultParams.BlsSignature,
Phase: avstypes.Phase(resultParams.Phase),
+ SubmittedAt: uint64(ctx.BlockTime().Unix()),
}Likely invalid or redundant comment.
The `runTx` ensures that, upon a message failure, the cache is not written to disk.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
precompiles/avs/tx.go (1)
380-382: Update phase documentation.
The comments should reflect the actual phase values used in the implementation:
// The phase of the Two-Phase Commit protocol:
-// 1 = Prepare phase (commit preparation)
-// 2 = Commit phase (final commitment)
+// Phase 1: Prepare phase (commit preparation)
+// Phase 2: Commit phase (final commitment)📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/avs/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
- precompiles/avs/tx.go (11 hunks)
- proto/exocore/avs/v1/tx.proto (12 hunks)
- x/avs/client/cli/tx.go (5 hunks)
- x/avs/keeper/multi_operator_submit_task_test.go (3 hunks)
- x/avs/keeper/submit_task_test.go (5 hunks)
- x/avs/keeper/task.go (13 hunks)
- x/avs/types/utils.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- x/avs/keeper/multi_operator_submit_task_test.go
- x/avs/keeper/submit_task_test.go
- x/avs/keeper/task.go
🧰 Additional context used
🪛 GitHub Check: break-check
proto/exocore/avs/v1/tx.proto
[failure] 231-231:
Field "7" with name "phase" on message "TaskResultInfo" changed option "json_name" from "stage" to "phase".
[failure] 231-231:
Field "7" on message "TaskResultInfo" changed type from "string" to "enum".
[failure] 231-231:
Field "7" on message "TaskResultInfo" changed name from "stage" to "phase".
🔇 Additional comments (15)
x/avs/client/cli/tx.go (5)
22-22: LGTM: Consistent with phase-based terminology.
The rename from FlagStage to FlagPhase aligns with the new phase-based model.
57-60: LGTM: Proper error handling added.
The addition of error handling for newBuildMsg improves robustness.
90-90: LGTM: Phase flag correctly marked as required.
Making the phase flag required ensures this critical parameter is always provided.
107-115: LGTM: Robust error handling for hex decoding.
Proper error handling for hex decoding of task response and BLS signature prevents invalid data from being processed.
119-122: Verify phase validation implementation.
The phase validation looks good, but let's verify the implementation of ValidatePhase to ensure it correctly handles the valid values (0 and 1).
Also applies to: 131-131
✅ Verification successful
Let me gather more information about the phase constants to ensure we have a complete picture.
Phase validation implementation is correct and well-defined
The validation implementation correctly handles all valid phase values. The ValidatePhase function validates against the following well-defined phase constants:
PhaseUnspecified = 0PhasePrepare = 1PhaseDoCommit = 2
The validation logic in x/avs/types/utils.go appropriately accepts only PhasePrepare and PhaseDoCommit as valid values, returning an error for any other phase value including PhaseUnspecified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the phase validation implementation
# Expected: Find the ValidatePhase function and confirm valid phase values
echo "Searching for ValidatePhase implementation..."
rg -A 10 "func ValidatePhase"
echo "Searching for Phase type definition..."
rg "type Phase"
Length of output: 616
Script:
#!/bin/bash
# Search for PhasePrepare and PhaseDoCommit constants
echo "Searching for phase constants..."
rg "Phase(Prepare|DoCommit)"
# Search for any other phase-related constants or enums
echo "Searching for other phase-related constants..."
rg "Phase.*=.*"
Length of output: 1976
x/avs/types/utils.go (2)
12-12:
Use consistent data type for BLS public key.
The BlsPublicKey field is defined as string while other cryptographic fields like PubKey in BlsParams use []byte. For consistency and to avoid unnecessary conversions, consider using []byte.
- BlsPublicKey string `json:"bls_public_key"`
+ BlsPublicKey []byte `json:"bls_public_key"`Likely invalid or redundant comment.
94-94:
Define phase constants.
The function references undefined constants PhasePrepare and PhaseDoCommit. These should be defined along with the Phase type.
const (
PhasePrepare Phase = iota // Prepare phase (commit preparation)
PhaseDoCommit // Commit phase (final commitment)
)⛔ Skipped due to learnings
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#220
File: precompiles/avs/tx.go:331-404
Timestamp: 2024-10-30T03:07:31.844Z
Learning: In the Two-Phase Commit protocol used in AVS, the phase values are 1 (Prepare) and 2 (Commit), corresponding to PreparePhase and DoCommitPhase.
proto/exocore/avs/v1/tx.proto (5)
47-47: LGTM! Improved formatting for gogoproto options
The alignment of gogoproto options improves readability.
Also applies to: 50-50
56-71: LGTM! Enhanced documentation clarity
The comments have been improved to be more descriptive and precise, particularly for:
- OperatorStatus fields
- RewardSlashProof message
- TaskInfo challenge period
Also applies to: 94-96
113-120: LGTM! Proper type definition for power-related fields
The power-related fields now correctly use the cosmos.Dec type with appropriate gogoproto options.
229-231: Breaking change: stage → phase transition requires migration
The change from string stage to Phase phase is a breaking change that affects message serialization and client compatibility. This has been flagged in previous reviews and requires careful migration handling.
#!/bin/bash
# Search for existing usages of TaskResultInfo.stage that need migration
rg "TaskResultInfo.*stage" --type go --type rust --type typescript
# Look for any persistence layers that might need migration
rg "TaskResultInfo.*Marshal" --type go🧰 Tools
🪛 GitHub Check: break-check
[failure] 231-231:
Field "7" with name "phase" on message "TaskResultInfo" changed option "json_name" from "stage" to "phase".
[failure] 231-231:
Field "7" on message "TaskResultInfo" changed type from "string" to "enum".
[failure] 231-231:
Field "7" on message "TaskResultInfo" changed name from "stage" to "phase".
252-252: LGTM! Improved RPC method signature formatting
The RPC method signatures have been reformatted to remove unnecessary spaces between the method name and parameter list, following standard protobuf style.
Also applies to: 256-256, 264-264
precompiles/avs/tx.go (3)
28-28: LGTM!
The new constant follows the existing naming convention and is properly aligned with other method constants.
Line range hint 31-329: LGTM!
The addition of stateDB parameter and event emission is consistent across all methods, with proper error handling.
398-398: 🛠️ Refactor suggestion
Consider using a constant for the key in SetTaskResultInfo.
Using the operator address as a key directly might be error-prone. Consider defining a constant or helper function to generate consistent keys.
+// generateTaskResultKey generates a consistent key for task results
+func (p Precompile) generateTaskResultKey(operatorAddr string) string {
+ return fmt.Sprintf("task_result:%s", operatorAddr)
+}
-err := p.avsKeeper.SetTaskResultInfo(ctx, resultParams.CallerAddress.String(), result)
+err := p.avsKeeper.SetTaskResultInfo(ctx, p.generateTaskResultKey(resultParams.CallerAddress.String()), result)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
x/avs/keeper/keeper.go (1)
66-69: Fix documentation comment.The comment incorrectly states "returns the operatorKeeper" when it should be "returns the epochsKeeper".
-// GetEpochKeeper returns the operatorKeeper from the Keeper struct. +// GetEpochKeeper returns the epochsKeeper from the Keeper struct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
x/avs/keeper/keeper.go(14 hunks)
🔇 Additional comments (3)
x/avs/keeper/keeper.go (3)
Line range hint 290-311: LGTM!
The changes improve error handling and maintain consistency in address string conversions.
420-422: LGTM!
The error handling for AVS address validation is properly implemented.
245-247: 🛠️ Refactor suggestion
Extract task ID format string into a constant.
For better maintainability and consistency, consider extracting the task ID format string into a constant.
+const (
+ TaskIDFormat = "%d"
+)
-if k.IsExistTask(ctx, strconv.FormatUint(params.TaskID, 10), params.TaskContractAddress.String()) {
+if k.IsExistTask(ctx, fmt.Sprintf(TaskIDFormat, params.TaskID), params.TaskContractAddress.String()) {Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
precompiles/avs/IAVSManager.sol (1)
165-181: Consider adding parameter validation documentationThe function is well-documented, but consider adding:
- Valid ranges or formats for
taskResponseandblsSignature- Expected behavior when invalid phase values are provided
- Any size limitations for the byte arrays
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
precompiles/avs/IAVSManager.sol(4 hunks)precompiles/avs/abi.json(7 hunks)precompiles/avs/events.go(2 hunks)x/avs/keeper/avs.go(6 hunks)x/avs/keeper/avs_test.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- precompiles/avs/abi.json
- x/avs/keeper/avs.go
🧰 Additional context used
📓 Learnings (2)
precompiles/avs/IAVSManager.sol (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#220
File: precompiles/avs/IAVSManager.sol:171-173
Timestamp: 2024-10-30T03:02:23.100Z
Learning: The phase parameter for the Two-Phase Commit protocol uses values 1 and 2, where 1 represents the Prepare phase and 2 represents the Commit phase.
x/avs/keeper/avs_test.go (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#220
File: precompiles/avs/avs_test.go:200-207
Timestamp: 2024-10-18T02:46:38.575Z
Learning: In AVS tests, use the `avsUnbondingPeriod` constant instead of hardcoded numbers for loop iteration counts to ensure consistency between registration and deregistration.
🔇 Additional comments (14)
x/avs/keeper/avs_test.go (5)
6-8: LGTM: Import changes are well-organized and necessary.
The new imports and alias changes improve code readability and support the new functionality.
Also applies to: 12-12
73-75: LGTM: Improved address handling in register test.
The changes enhance the robustness of the test by:
- Using proper address formatting with HexToAddress
- Implementing case-insensitive address comparison
- Maintaining consistency in UnbondingPeriod values
Also applies to: 79-80, 90-90
153-156: LGTM: Enhanced operator registration test coverage.
The changes improve test coverage by:
- Adding proper error handling for non-existent operator
- Testing the complete operator registration flow
- Setting up proper asset state for testing
Also applies to: 173-177
31-31: 🛠️ Refactor suggestion
Optimize epoch simulation in TestAVS.
The test simulates multiple epoch transitions with repeated CommitAfter calls. Consider using a constant for better maintainability:
+const avsUnbondingPeriod = 2
-suite.CommitAfter(48*time.Hour + time.Nanosecond)
-suite.CommitAfter(48*time.Hour + time.Nanosecond)
+for i := 0; i < avsUnbondingPeriod; i++ {
+ suite.CommitAfter(24*time.Hour + time.Nanosecond)
+ epoch, found := suite.App.EpochsKeeper.GetEpochInfo(suite.Ctx, epochstypes.DayEpochID)
+ suite.Equal(found, true)
+ suite.Equal(epoch.CurrentEpoch, int64(i+2))
+}Also applies to: 57-63
127-132: 🛠️ Refactor suggestion
Optimize epoch simulation loop in deregister test.
The current implementation uses a hardcoded value. Consider using the UnbondingPeriod constant for consistency:
-for epochEnd := epoch.CurrentEpoch; epochEnd <= int64(info.Info.StartingEpoch)+2; epochEnd++ {
+const avsUnbondingPeriod = 2
+for i := 0; i < avsUnbondingPeriod; i++ {
suite.CommitAfter(time.Hour * 24)
epoch, found := suite.App.EpochsKeeper.GetEpochInfo(suite.Ctx, epochstypes.DayEpochID)
suite.Equal(found, true)
- suite.Equal(epoch.CurrentEpoch, epochEnd+1)
+ suite.Equal(epoch.CurrentEpoch, epoch.CurrentEpoch+int64(i)+1)
}precompiles/avs/events.go (6)
4-23: LGTM! Event type constants and imports are well-organized.
The event type constants follow a consistent naming convention and the imports cover all required functionality.
26-43: LGTM! Well-implemented generic event helper.
The emitEvent helper function effectively reduces code duplication and properly handles event emission logic, including topics and data packing.
Line range hint 200-236: Verify argument count and consider using emitEvent helper.
The task creation event has complex argument handling. Let's verify:
- The argument count matches event inputs
- Consider using the
emitEventhelper for consistency
#!/bin/bash
# Verify event input count matches packed arguments
echo "=== Checking TaskCreated event inputs ==="
rg -p "TaskCreated.*event" -A 10Example refactor using emitEvent:
func (p Precompile) EmitTaskCreated(ctx sdk.Context, stateDB vm.StateDB, task *avstypes.TaskInfoParams) error {
- // Current implementation
+ arguments := p.ABI.Events[EventTypeTaskCreated].Inputs
+ return p.emitEvent(ctx, stateDB, EventTypeTaskCreated, arguments,
+ task.CallerAddress.String(), task.TaskName, task.Hash,
+ task.TaskResponsePeriod, task.TaskChallengePeriod,
+ task.ThresholdPercentage, task.TaskStatisticalPeriod)
}238-253: Verify argument count for challenge and key events.
The implementation uses the emitEvent helper correctly, but let's verify the argument counts match their respective event inputs.
✅ Verification successful
Event argument counts match their definitions
The verification confirms that both event implementations correctly match their Solidity definitions:
- ChallengeInitiated: Takes 5 arguments (sender, taskHash, taskID, taskResponseHash, operatorAddress) in both the event definition and emit call
- PublicKeyRegistered: Takes 2 arguments (sender, name) in both the event definition and emit call
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify event input counts
echo "=== Checking ChallengeInitiated event inputs ==="
rg -p "ChallengeInitiated.*event" -A 10
echo "=== Checking PublicKeyRegistered event inputs ==="
rg -p "PublicKeyRegistered.*event" -A 10
Length of output: 257
Script:
#!/bin/bash
# Let's try a different approach to find event definitions
# Search for event type constants
echo "=== Event Type Constants ==="
rg "EventType(ChallengeInitiated|PublicKeyRegistered)"
# Search for event definitions in ABI
echo "=== Event Definitions in ABI ==="
rg -A 5 '(ChallengeInitiated|PublicKeyRegistered).*\{.*"type":\s*"event"'
# Search for any solidity event definitions
echo "=== Solidity Event Definitions ==="
fd -e sol | xargs rg "event (ChallengeInitiated|PublicKeyRegistered)"
Length of output: 1125
Script:
#!/bin/bash
# Let's examine the full event definitions and their implementations
# Get the full ChallengeInitiated event definition
echo "=== ChallengeInitiated Event Definition ==="
rg -A 5 "event ChallengeInitiated" precompiles/avs/IAVSManager.sol
# Get the full PublicKeyRegistered event definition
echo "=== PublicKeyRegistered Event Definition ==="
rg -B 1 -A 1 "event PublicKeyRegistered" precompiles/avs/IAVSManager.sol
# Get the emitEvent helper implementation
echo "=== EmitEvent Helper Implementation ==="
rg -A 10 "func.*emitEvent" precompiles/avs/events.go
Length of output: 1203
142-198: 🛠️ Refactor suggestion
Refactor operator events and verify address handling.
- The operator event functions contain duplicated logic that could be simplified.
- Based on past review comments, ensure operator addresses are handled as Cosmos AccAddress.
Example refactor for EmitOperatorJoined (apply similar pattern to EmitOperatorOuted):
func (p Precompile) EmitOperatorJoined(ctx sdk.Context, stateDB vm.StateDB, params *avstypes.OperatorOptParams) error {
- event := p.ABI.Events[EventTypeOperatorJoined]
- topics := make([]common.Hash, 2)
- // ... rest of the implementation
+ arguments := p.ABI.Events[EventTypeOperatorJoined].Inputs
+ return p.emitEvent(ctx, stateDB, EventTypeOperatorJoined, arguments,
+ params.OperatorAddress.String())
}Let's verify the address handling:
✅ Verification successful
Let me gather more information about the OperatorOptParams struct definition.
Based on the gathered information, I can now generate the final response:
Refactoring suggestion is valid, but address handling is already correct
The operator events can be refactored to use the emitEvent helper to reduce code duplication. However, the address handling is already correctly implemented as the OperatorOptParams.OperatorAddress is properly defined as sdk.AccAddress and consistently used across the codebase.
Example refactor:
func (p Precompile) EmitOperatorJoined(ctx sdk.Context, stateDB vm.StateDB, params *avstypes.OperatorOptParams) error {
return p.emitEvent(ctx, stateDB, EventTypeOperatorJoined,
p.ABI.Events[EventTypeOperatorJoined].Inputs[1:],
params.AvsAddress,
params.OperatorAddress.String())
}
func (p Precompile) EmitOperatorOuted(ctx sdk.Context, stateDB vm.StateDB, params *avstypes.OperatorOptParams) error {
return p.emitEvent(ctx, stateDB, EventTypeOperatorLeft,
p.ABI.Events[EventTypeOperatorLeft].Inputs[1:],
params.AvsAddress,
params.OperatorAddress.String())
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how OperatorAddress is defined and used
echo "=== Checking OperatorAddress type ==="
rg -p "type OperatorOptParams struct" -A 10
rg -p "OperatorAddress.*String\(\)" -A 2
Length of output: 6803
Script:
#!/bin/bash
# Check OperatorOptParams struct definition
echo "=== Checking OperatorOptParams struct ==="
rg -p "type OperatorOptParams struct" -A 5
# Check if there are any other relevant operator address type definitions
echo "=== Checking operator address type definitions ==="
ast-grep --pattern 'type $_ struct {
OperatorAddress $_
$$$
}'
Length of output: 2028
255-286: 🛠️ Refactor suggestion
Verify argument count and refactor using emitEvent helper.
The task submission event could be simplified using the emitEvent helper while maintaining the same functionality.
Example refactor:
func (p Precompile) EmitTaskSubmittedByOperator(ctx sdk.Context, stateDB vm.StateDB, params *avstypes.TaskResultParams) error {
- // Current implementation
+ arguments := p.ABI.Events[EventTypeTaskSubmittedByOperator].Inputs
+ return p.emitEvent(ctx, stateDB, EventTypeTaskSubmittedByOperator, arguments,
+ params.CallerAddress.String(), params.TaskResponse,
+ params.BlsSignature, params.Phase)
}precompiles/avs/IAVSManager.sol (3)
7-7: LGTM: Constants and interface declaration
The addition of the AVSMANAGER_CONTRACT constant provides a convenient way to interact with the precompile contract, and the empty line improves code organization.
Also applies to: 12-12
14-42: LGTM: Event declarations
The events are well-structured and follow the established patterns:
- Sender parameter uses string type to accommodate bech32 addresses
- Indexed keyword is correctly used only for address and uint types
- Parameters are logically organized
213-217:
Fix incomplete return value documentation
The documentation for getTaskInfo return value is missing some array elements. Update it to include all returned values in the correct order:
/// @return info Array containing task information in the following order:
/// [0] = startingEpochNumber
/// [1] = taskResponsePeriod
-/// [2] = taskStatisticalPeriod
+/// [2] = taskChallengePeriod
+/// [3] = thresholdPercentage
+/// [4] = taskStatisticalPeriodLikely invalid or redundant comment.
|
Testing. |
|
The AVS interface in precompiles/avs/query.go needs to convert AVS addresses to lowercase |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
x/operator/keeper/operator.go (2)
Line range hint
192-211: Consider normalizing AVS address inGetOptedInAVSForOperator.This method returns AVS addresses directly from storage without case normalization. While the storage keys are now normalized, consider normalizing the returned addresses for consistency with the new changes.
Apply this diff:
func (k *Keeper) GetOptedInAVSForOperator(ctx sdk.Context, operatorAddr string) ([]string, error) { // get all opted-in info store := prefix.NewStore(ctx.KVStore(k.storeKey), operatortypes.KeyPrefixOperatorOptedAVSInfo) iterator := sdk.KVStorePrefixIterator(store, []byte(operatorAddr)) defer iterator.Close() avsList := make([]string, 0) for ; iterator.Valid(); iterator.Next() { keys, err := assetstype.ParseJoinedStoreKey(iterator.Key(), 2) if err != nil { return nil, err } - avsList = append(avsList, keys[1]) + avsList = append(avsList, strings.ToLower(keys[1])) } return avsList, nil }
Line range hint
229-249: Consider normalizing AVS address comparison inGetOptedInOperatorListByAVS.The AVS address comparison should be case-insensitive to match the new storage pattern.
Apply this diff:
func (k *Keeper) GetOptedInOperatorListByAVS(ctx sdk.Context, avsAddr string) ([]string, error) { + normalizedAVSAddr := strings.ToLower(avsAddr) // get all opted-in info store := prefix.NewStore(ctx.KVStore(k.storeKey), operatortypes.KeyPrefixOperatorOptedAVSInfo) iterator := sdk.KVStorePrefixIterator(store, nil) defer iterator.Close() operatorList := make([]string, 0) for ; iterator.Valid(); iterator.Next() { keys, err := assetstype.ParseJoinedStoreKey(iterator.Key(), 2) if err != nil { return nil, err } - if avsAddr == keys[1] { + if normalizedAVSAddr == strings.ToLower(keys[1]) { operatorList = append(operatorList, keys[0]) } } return operatorList, nil }precompiles/avs/tx.go (1)
394-402: Consider adding timestamp for audit purposes.For better auditability and tracking, consider adding a timestamp to the TaskResultInfo struct to record when the result was submitted.
result := &avstypes.TaskResultInfo{ TaskId: resultParams.TaskID, OperatorAddress: resultParams.CallerAddress.String(), TaskContractAddress: resultParams.TaskContractAddress.String(), TaskResponse: resultParams.TaskResponse, BlsSignature: resultParams.BlsSignature, Phase: phaseEnum, + SubmittedAt: uint64(ctx.BlockTime().Unix()), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
precompiles/avs/query.go(7 hunks)precompiles/avs/tx.go(11 hunks)x/operator/keeper/operator.go(4 hunks)
🧰 Additional context used
📓 Learnings (1)
precompiles/avs/query.go (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#220
File: precompiles/avs/query.go:188-191
Timestamp: 2024-10-29T05:39:16.900Z
Learning: In this codebase, `uint64` is intentionally used for `taskID` parameters, and we should not suggest changing it to `*big.Int` or `uint256` without valid reasons.
🔇 Additional comments (12)
precompiles/avs/query.go (5)
4-8: LGTM: Import changes and new method constants.
The new imports and method constants are well-organized and follow the existing patterns.
Also applies to: 26-29
45-52: LGTM: Improved error handling in GetRegisteredPubkey.
The error handling now properly uses errors.Is and consistently returns an empty byte array when the key is not found.
70-70: LGTM: Consistent error handling and case-insensitive address handling.
The changes ensure consistent error handling across methods and proper case-insensitive address handling.
Also applies to: 93-95, 121-123
206-225: LGTM: Well-implemented GetCurrentEpoch method.
The method is well-documented, has proper input validation, and appropriate error handling.
195-196:
Fix incorrect comment.
The comment refers to AVS non-existence, but the code is handling task non-existence.
- // if the avs does not exist, return empty array
+ // if the task does not exist, return empty stringLikely invalid or redundant comment.
x/operator/keeper/operator.go (2)
110-110: LGTM: Consistent case normalization for AVS addresses.
The changes correctly normalize AVS addresses to lowercase when generating store keys, addressing the audit finding TOW-02 and preventing case-sensitivity issues.
Also applies to: 134-134, 147-147
Line range hint 1-249: Consider data migration for existing mixed-case AVS addresses.
The changes ensure new AVS addresses are stored in lowercase, but existing data might contain mixed-case addresses. Consider adding a migration to normalize existing keys.
Let's check if there are any existing mixed-case AVS addresses in the store:
precompiles/avs/tx.go (5)
28-28: LGTM: New method constant added.
The constant MethodOperatorSubmitTask is correctly defined and follows the existing naming convention.
31-36: LGTM: StateDB parameter consistently added.
The stateDB vm.StateDB parameter has been consistently added to all precompile methods to support event emissions.
Also applies to: 68-68, 105-105, 140-140, 170-170, 224-224, 283-283
58-60: LGTM: Event emissions added.
Event emissions have been consistently added to all state-changing operations, improving the observability of the system.
Also applies to: 95-97, 130-132, 160-162, 189-191, 213-215, 272-274, 325-327, 407-409
363-365: LGTM: Proper handling of empty task response.
The code correctly handles empty task response by setting it to nil, preventing potential issues with empty byte arrays.
379-392: LGTM: Phase validation is correct.
The phase validation correctly enforces the Two-Phase Commit protocol values (1 for Prepare, 2 for Commit).
|
Operator opt-in the new registered avs |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
x/operator/keeper/operator_slash_state.go (2)
72-72: LGTM with suggestion: Consider adding address validation.The address normalization is correctly implemented. However, consider adding address validation similar to
UpdateOperatorSlashInfoto ensure input validity before processing.Consider adding this validation at the start of the function:
func (k *Keeper) AllOperatorSlashInfo(ctx sdk.Context, avsAddr, operatorAddr string) (map[string]*operatortypes.OperatorSlashInfo, error) { + // Validate operator address + if _, err := sdk.AccAddressFromBech32(operatorAddr); err != nil { + return nil, assetstype.ErrInvalidOperatorAddr + } store := prefix.NewStore(ctx.KVStore(k.storeKey), operatortypes.KeyPrefixOperatorSlashInfo)
Line range hint
134-134: Address architectural decision for client chain interaction.The TODO comment indicates uncertainty about how to send slash info to the client chain. This architectural decision should be addressed before it becomes a technical debt.
Consider:
- Document the tradeoffs of using EndBlock vs alternatives
- Create a tracking issue for this architectural decision
Would you like me to create a GitHub issue to track this architectural decision?
x/operator/keeper/usd_value.go (1)
Line range hint
366-371: Critical: Unsafe error handling for missing price rounds.The current implementation ignores price round not found errors and defaults to a price of 1, which:
- Could lead to incorrect USD value calculations
- May affect slashing calculations and voting power updates
- Is marked as a temporary solution with a TODO comment
This requires proper error handling to ensure accurate financial calculations.
Consider implementing one of these approaches:
- Fail the operation when price data is missing
- Implement a price oracle fallback mechanism
- Add configuration for handling missing price scenarios
Would you like me to help create a GitHub issue to track this improvement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
x/operator/keeper/grpc_query.go(2 hunks)x/operator/keeper/impl_epoch_hook.go(1 hunks)x/operator/keeper/operator.go(5 hunks)x/operator/keeper/operator_slash_state.go(4 hunks)x/operator/keeper/usd_value.go(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/operator/keeper/impl_epoch_hook.go
- x/operator/keeper/operator.go
🧰 Additional context used
📓 Learnings (1)
x/operator/keeper/usd_value.go (1)
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#222
File: x/operator/keeper/usd_value.go:273-273
Timestamp: 2024-11-03T13:18:31.026Z
Learning: In the Exocore codebase, all store keys are stored in lowercase throughout the system. Therefore, converting keys to lowercase in `SetAllOperatorUSDValues` in `x/operator/keeper/usd_value.go` is acceptable without requiring migration logic.
🔇 Additional comments (8)
x/operator/keeper/operator_slash_state.go (3)
4-5: LGTM: Import addition is appropriate.
The addition of the strings package is necessary for the address normalization changes.
59-59: LGTM: Consistent address normalization in retrieval.
The change ensures that slash info retrieval uses the same normalized address format as storage, maintaining consistency.
28-28: LGTM: Address normalization prevents case-sensitivity issues.
Converting AVS addresses to lowercase ensures consistent key generation regardless of address case. This is a good security practice.
Let's verify that all AVS addresses are consistently normalized across the codebase:
✅ Verification successful
Based on the search results, I can see that AVS addresses are consistently normalized to lowercase across the codebase using strings.ToLower() in all key generation and storage operations. Specifically:
- All
GetJoinedStoreKeycalls involving AVS addresses usestrings.ToLower(avsAddr) - The AVS address generation itself in
GenerateAVSAddrreturns lowercase addresses - All storage operations in keeper methods normalize AVS addresses before storing
- Query handlers and CLI commands normalize addresses before querying
The change in the review comment aligns with the existing pattern in the codebase.
LGTM: AVS address normalization is consistent across the codebase
The change to use strings.ToLower() for AVS addresses in key generation matches the established pattern used throughout the codebase for handling AVS addresses. This ensures consistent case-insensitive key generation and storage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any AVS address handling that might not be normalized
# Look for potential AVS address operations that might miss normalization
# Search for AVS address related operations
rg -n "avsAddr|AVSAddr" --type go
# Search for potential direct key generation without ToLower
rg -n "GetJoinedStoreKey.*avsAddr" --type go
Length of output: 56900
x/operator/keeper/grpc_query.go (3)
6-6: LGTM: Import addition is correct.
The strings import is properly placed in the standard library imports section and is required for the address normalization changes.
207-207: Clarify data migration strategy for existing AVS addresses.
Given that this change normalizes AVS addresses to lowercase, we need to ensure that existing data with mixed-case addresses can still be accessed. Please clarify:
- Are there existing records with mixed-case addresses?
- Is a data migration strategy needed?
- Should we add a migration script to normalize existing data?
Let's check for mixed-case addresses in the existing data:
#!/bin/bash
# Search for potential AVS addresses in the codebase to identify case patterns
echo "Searching for potential AVS addresses..."
rg -i "0x[a-f0-9]{40}" --type go207-207: Verify consistent address normalization across the codebase.
While converting the AVS address to lowercase is correct, we should ensure this change is consistently applied across all related functions to prevent mixed-case issues.
Let's verify the consistency:
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information about AVS address handling across the codebase.
AVS address normalization is consistently implemented
The codebase already handles AVS addresses consistently by converting them to lowercase across different components:
- Operator module:
strings.ToLower(req.AvsAddress)in query handling - AVS module:
- Task operations: Multiple instances of
strings.ToLower(taskContractAddress) - AVS info storage:
strings.ToLower(params.AvsAddress.String()) - Operator management:
strings.ToLower(params.AvsAddress.String())in opt-in/opt-out operations
- Task operations: Multiple instances of
The change aligns with the existing pattern of case normalization throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AVS address handling patterns across the codebase
echo "Searching for AVS address usage patterns..."
rg -A 2 "AvsAddress|avsAddress|AVSAddress" --type go
# Search for ToLower usage with addresses
echo "Checking ToLower usage with addresses..."
rg "strings\.ToLower.*[Aa]ddress" --type go
Length of output: 21857
x/operator/keeper/usd_value.go (2)
6-6: LGTM: Import addition is appropriate.
The addition of the "strings" package import is necessary for the case normalization changes.
31-31: LGTM: Consistent case normalization of AVS addresses.
The changes consistently normalize AVS addresses to lowercase when constructing store keys, which:
- Aligns with the system-wide practice of using lowercase store keys
- Ensures case-insensitive handling of AVS addresses
- Prevents potential key mismatches due to case differences
Based on previous learnings, this change is safe and doesn't require migration logic.
Also applies to: 66-66, 91-91, 99-99, 130-130, 151-151, 173-173, 182-182, 196-196, 210-210
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
precompiles/avs/events.go (1)
255-286: Refactor EmitTaskSubmittedByOperator to use the generic helper.Consider simplifying this function using the generic
emitEventhelper:func (p Precompile) EmitTaskSubmittedByOperator(ctx sdk.Context, stateDB vm.StateDB, params *avstypes.TaskResultParams) error { - event := p.ABI.Events[EventTypeTaskSubmittedByOperator] - // ... existing implementation ... + arguments := p.ABI.Events[EventTypeTaskSubmittedByOperator].Inputs + return p.emitEvent(ctx, stateDB, EventTypeTaskSubmittedByOperator, arguments, + params.CallerAddress.String(), params.TaskResponse, + params.BlsSignature, uint8(params.Phase)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
precompiles/avs/events.go(2 hunks)
🔇 Additional comments (3)
precompiles/avs/events.go (3)
15-23: LGTM! Well-organized event type constants.
The event type constants follow consistent naming conventions and clearly represent their purposes.
238-253: LGTM! Good use of the generic emitEvent helper.
These functions demonstrate clean implementation using the generic helper.
Line range hint 200-236: Verify argument count in EmitTaskCreated.
The number of arguments in Pack should match the number of inputs selected:
Also consider refactoring to use the generic emitEvent helper:
func (p Precompile) EmitTaskCreated(ctx sdk.Context, stateDB vm.StateDB, task *avstypes.TaskInfoParams) error {
- event := p.ABI.Events[EventTypeTaskCreated]
- // ... existing implementation ...
+ arguments := p.ABI.Events[EventTypeTaskCreated].Inputs
+ return p.emitEvent(ctx, stateDB, EventTypeTaskCreated, arguments,
+ task.CallerAddress.String(), task.TaskName, task.Hash,
+ task.TaskResponsePeriod, task.TaskChallengePeriod,
+ task.ThresholdPercentage, task.TaskStatisticalPeriod)
}✅ Verification successful
Event argument count matches the implementation correctly
Looking at the TaskCreated event definition, it has 9 parameters total:
- 2 indexed parameters (taskContractAddress, taskId) handled by topics[1] and topics[2]
- 7 non-indexed parameters (sender, name, hash, taskResponsePeriod, taskChallengePeriod, thresholdPercentage, taskStatisticalPeriod) handled by the
Packcall
The implementation correctly selects inputs[2] through inputs[8] (7 parameters) for packing, which matches the non-indexed parameters in the event definition. The argument count is accurate and properly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the event definition matches implementation
echo "=== Checking TaskCreated event definition ==="
rg "TaskCreated.*event" precompiles/avs/IAVSManager.sol -A 5
Length of output: 163
Script:
#!/bin/bash
# Let's try searching for TaskCreated event in all Solidity files
echo "=== Checking TaskCreated event definition in all Solidity files ==="
fd -e sol . | xargs rg "event\s+TaskCreated"
# Also check the event type constant in Go files
echo -e "\n=== Checking EventTypeTaskCreated constant ==="
rg "EventTypeTaskCreated\s*=\s*" precompiles/avs/
# Let's also check the ABI initialization to see event registration
echo -e "\n=== Checking ABI initialization ==="
rg "RegisterEventABI|NewABI.*TaskCreated" precompiles/avs/
Length of output: 631
Script:
#!/bin/bash
# Let's get the full TaskCreated event definition with its parameters
echo "=== Full TaskCreated event definition ==="
rg "event TaskCreated" precompiles/avs/IAVSManager.sol -A 10
# Let's also check the event registration in the ABI
echo -e "\n=== Checking event registration in ABI ==="
rg "TaskCreated.*\{.*\}" precompiles/avs/ -A 5
Length of output: 619
|
Test passed with 0c527a1, check all these events emission successfully. |
Description
1.add missing events
2.add missing pre compiled interfaces
3.fix audit TOW-02
Summary by CodeRabbit
New Features
AVSRegistered,AVSUpdated, andTaskSubmittedByOperator.getAVSInfo,getTaskInfo, andisOperator.operatorSubmitTaskfunction.Phaseenum to enhance clarity in task submission phases.Bug Fixes
Documentation
Refactor
Tests