Skip to content

[DO NOT MERGE]: slashOperator appointees - #1710

Draft
ypatil12 wants to merge 2 commits into
mainfrom
script/slasher-appointees
Draft

[DO NOT MERGE]: slashOperator appointees#1710
ypatil12 wants to merge 2 commits into
mainfrom
script/slasher-appointees

Conversation

@ypatil12

@ypatil12 ypatil12 commented Jan 27, 2026

Copy link
Copy Markdown
Contributor

Script that gets the appointees for the slashOperator function for all avss. . Currently there are none:

Total operator sets in input: 90
Unique AVS addresses: 33
Summary:
Total operator sets: 90
Unique AVS addresses: 33
AVS with slasher appointees: 0
Total slasher appointees: 0

@ypatil12 ypatil12 changed the title [DO NOT MERGE] [DO NOT MERGE]: slashOperator appointees Jan 27, 2026
@nadir-akhtar

Copy link
Copy Markdown
Collaborator

@claude [test] Provide input on this PR

@claude

claude Bot commented Feb 2, 2026

Copy link
Copy Markdown

Claude encountered an error after 1m 25s —— View job


Review Progress

  • Read and analyze changed files
  • Review Solidity script implementations
  • Provide feedback and recommendations

Code Review Feedback

I've reviewed the PR and have the following observations and recommendations:

Overall Assessment

The scripts are well-structured and serve their purpose of querying appointees and migrating slashers for operator sets. The implementation is straightforward and follows the expected patterns. Here are my detailed findings:


get_operatorSet_appointees.s.sol

Strengths:

  • Clear documentation with usage examples (lines 10-25)
  • Proper error handling with view-only function
  • Good logging output for tracking progress
  • Efficient deduplication logic in _getUniqueAvs (lines 96-127)

Recommendations:

  1. Missing Import (line 56): The script uses IAllocationManagerActions.slashOperator.selector but only imports IAllocationManager. While this works if IAllocationManager inherits from IAllocationManagerActions, it would be clearer to verify the import structure.

  2. Gas Considerations: The _getUniqueAvs function has O(n²) complexity due to nested loops (lines 103-118). For the current dataset (90 operator sets → 33 unique AVS), this is fine, but consider using a more efficient deduplication approach if dealing with larger datasets.

  3. Sanity Check Comment (line 24-25): The comment mentions verifying with a different selector, but it's hardcoded to a specific AVS/operatorSet. Consider adding a more generalizable example or making it a separate function for testing purposes.


migrate_slashers.s.sol

Strengths:

  • Proper batching logic to handle gas limits (BATCH_SIZE = 20)
  • Good use of vm.startBroadcast()/vm.stopBroadcast() for transaction control
  • Clear logging for each batch
  • Safe type casting from uint256 to uint32 for operator set IDs (line 62)

Concerns & Recommendations:

  1. ⚠️ Critical: Type Casting Safety (line 62):

    operatorSets[i] = OperatorSet({avs: operatorSetData[i].avs, id: uint32(operatorSetData[i].id)});

    The script casts uint256 to uint32 without bounds checking. If any ID in the JSON is > type(uint32).max (4,294,967,295), this will silently truncate and cause incorrect behavior. Add validation:

    require(operatorSetData[i].id <= type(uint32).max, "Operator set ID exceeds uint32 max");
  2. Duplicate Struct Definition: Both scripts define OperatorSetData with identical structure (lines 39-43 in get_operatorSet_appointees.s.sol and lines 39-42 in migrate_slashers.s.sol). Consider extracting to a shared base contract or library to maintain DRY principle.

  3. No Dry-Run Verification: The migrate script could benefit from a pre-flight check (similar to the appointees script) that runs in view-only mode to verify:

    • Which operator sets will actually be migrated (not already migrated)
    • What the slasher will be set to for each
    • Gas estimation for each batch
  4. Batch Size Justification: The BATCH_SIZE = 20 (line 34) is hardcoded. According to the migrateSlashers implementation comments (AllocationManager.sol:367-368), "Gas cost is O(appointees) per operator set". Since your query shows 0 appointees currently, 20 is safe, but document the reasoning or make it configurable for future use.


operatorSets.json

Observations:

  • Contains 90 operator sets from 33 unique AVS addresses
  • All entries use id: 0 except for a few AVSs with multiple sets
  • Largest number of sets is for AVS 0x115c1d01133337f311c793c8ea1f8b6f11415500 (up to ID 32)
  • AVS 0xf32d566cc95d4a7b3e1ef211c1373f729ca18c86 has up to ID 27

Recommendations:

  1. Add a schema comment at the top of the JSON file documenting the expected structure
  2. Consider adding metadata like fetch timestamp or block number for auditing purposes

script/input/.gitkeep

No issues - standard practice for preserving empty directories.


General Recommendations

  1. Testing: Consider adding a test script that:

    • Validates the JSON structure before running the migration
    • Simulates the migration in a fork test environment
    • Verifies the gas costs for actual mainnet data
  2. Documentation: Add a README in script/tasks/ explaining:

    • The relationship between these two scripts
    • When to use each script
    • Expected outcomes and how to interpret results
    • Deprecation timeline (mentioned in IAllocationManager.sol:497 - "Early Q2 2026")
  3. Error Handling: While migrateSlashers is designed to not revert (as per IAllocationManager.sol:498-500), the script should still:

    • Log which operator sets were skipped and why
    • Report final statistics on successful vs skipped migrations
  4. Script Organization: Following the CLAUDE.md conventions, ensure:

    • Scripts follow conventional commit standards when merged
    • PR description follows the motivation/modifications/result format

Summary

The code is functional and serves its immediate purpose well. The main action item is adding bounds checking for the uint32 cast to prevent silent failures. The scripts could benefit from enhanced logging and pre-flight validation, but given the current state (0 appointees), they should work correctly for the immediate use case.

Verdict: Approve with recommendation to add the uint32 bounds check before production use.


|

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants