fix(avs): prevent BLS12-381 replay attacks #314
Conversation
WalkthroughThe pull request updates the BLS public key registration process by modifying the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (
|
trestinlsd
changed the title
trestinlsd
force-pushed
the
fix-issuse-304
branch
from
345c673 to
16b8823
Compare
trestinlsd
requested review from
MaxMustermann2,
magj2006,
bwhour,
cloud8little,
mikebraver,
TimmyExogenous and
leonz789
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
x/avs/keeper/keeper.go (3)
296-299: Favor Cosmos SDK error handling over fmt.Errorf.
Instead of returning a plain Go error, consider usingerrorsmod.Wrapor a similar Cosmos SDK error code to improve debugging and streamline handling in higher layers.- return fmt.Errorf("invalid message format: %s", params.Message) + return errorsmod.Wrap(types.ErrInvalidInput, fmt.Sprintf("invalid message format: %s", params.Message))
300-307: Ensure msgHash is exactly 32 bytes before converting to [32]byte.
While this code is correct assuming the hash is always 32 bytes, adding a length check can safeguard against malformed input.+ if len(msgHash) != 32 { + return errorsmod.Wrap(types.ErrInvalidHash, "PubKeyRegistrationMessageHash must be 32 bytes") + } expectedHash := crypto.Keccak256Hash([]byte(expectedMessage)) if !bytes.Equal(msgHash, expectedHash.Bytes()) { ... }
325-326: Grammar fix for the error message.
The error string has a minor grammatical issue.- return errorsmod.Wrap(types.ErrAlreadyExists, fmt.Sprintf("the bls key is already exists:%s", blsInfo.PubKey)) + return errorsmod.Wrap(types.ErrAlreadyExists, fmt.Sprintf("the BLS key already exists: %s", blsInfo.PubKey))x/avs/keeper/avs_test.go (1)
202-222: Consider expanding test coverage.
The current test succeeds with matching fields. You could add negative tests (e.g., mismatched messages, invalid signatures) to confirm that the registration fails as expected.precompiles/avs/IAVSManager.sol (1)
198-200: Documentation needs improvement.The documentation for the parameters is not descriptive enough:
pubKeyRegistrationSignatureis described as "the public keys of the operator" which is incorrectpubKeyRegistrationMessageHashis described as "the public keys of the operator" which is incorrectApply this diff to improve the documentation:
- /// @param pubKeyRegistrationSignature the public keys of the operator - /// @param pubKeyRegistrationMessageHash the public keys of the operator - /// @param msg is the message which signed using a private key + /// @param pubKeyRegistrationSignature the BLS signature of the registration message + /// @param pubKeyRegistrationMessageHash the hash of the registration message + /// @param msg the message that was signed using the private key
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
precompiles/avs/IAVSManager.sol(1 hunks)precompiles/avs/tx.go(1 hunks)x/avs/keeper/avs_test.go(2 hunks)x/avs/keeper/keeper.go(2 hunks)x/avs/types/utils.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-unit-cover
- GitHub Check: test-unit-e2e
- GitHub Check: Analyze (go)
- GitHub Check: goreleaser
🔇 Additional comments (11)
x/avs/keeper/keeper.go (4)
291-295: Good documentation to prevent replay attacks.
This comment block clearly explains the usage of theMessageparameter to avoid replay attacks by including chain ID and operator address in lowercase.
308-311: Confirm [32]byte conversion aligns with the BLS library requirements.
IfmsgHashis guaranteed to be 32 bytes, this invocation is valid. Otherwise, there could be a silent mismatch.
317-317: Struct naming clarity.
Renaming toblsInfoadds clarity by reflecting the purpose of this struct.
328-328: No issues found.
This return statement appropriately delegates to set the operator’s public key.x/avs/types/utils.go (1)
45-47: Field naming improves consistency, and addingMessageis beneficial.
RenamingPubkeyRegistrationSignature→PubKeyRegistrationSignatureandPubkeyRegistrationMessageHash→PubKeyRegistrationMessageHashaligns with standard casing. Including theMessagefield clarifies its role in preventing replay attacks.x/avs/keeper/avs_test.go (1)
4-6: No concerns regarding new imports.precompiles/avs/IAVSManager.sol (2)
201-208: LGTM! Parameter naming and structure improvements.The changes improve consistency in parameter naming by standardizing the casing of
pubKeyacross all parameters. The addition of themsgparameter enhances the registration process by providing the original message for signature verification.
205-207: LGTM! Parameter naming and structure changes look good.The parameter names have been updated for consistency, and the new
msgparameter has been added to provide additional data for signature validation.precompiles/avs/tx.go (3)
325-329: LGTM! Robust parameter validation.The implementation properly validates the new
msgparameter and correctly assigns it to theblsParamsstructure. The error handling follows the established pattern used for other parameters.
317-318: LGTM! Field name changes look good.The field names have been updated for consistency with the interface changes.
Also applies to: 323-324
325-329: LGTM! Parameter validation and field assignment look good.The new
msgparameter is properly validated and assigned to theMessagefield.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
precompiles/avs/tx.go (1)
317-318: Fix formatting issue.Remove the extra newline to comply with
gofumptformatting rules.blsParams.PubKeyRegistrationSignature = pubKeyRegistrationSignature -🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 318-318:
File is notgofumpt-ed (gofumpt)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
precompiles/avs/IAVSManager.sol(1 hunks)precompiles/avs/abi.json(1 hunks)precompiles/avs/tx.go(1 hunks)x/avs/keeper/avs_test.go(2 hunks)x/avs/keeper/keeper.go(1 hunks)x/avs/types/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- x/avs/types/utils.go
- precompiles/avs/IAVSManager.sol
- x/avs/keeper/keeper.go
- x/avs/keeper/avs_test.go
🧰 Additional context used
🪛 GitHub Check: Run golangci-lint
precompiles/avs/tx.go
[failure] 318-318:
File is not gofumpt-ed (gofumpt)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-unit-e2e
- GitHub Check: test-unit-cover
- GitHub Check: goreleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
precompiles/avs/tx.go (1)
284-328: LGTM! The changes align with the PR objectives.The function has been successfully refactored to improve BLS signature validation logic by:
- Removing the redundant
PubkeyRegistrationMessageHashparameter- Updating field names for consistency with Go naming conventions
- Maintaining robust parameter validation and error handling
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 318-318:
File is notgofumpt-ed (gofumpt)precompiles/avs/abi.json (1)
1017-1027: Ensure Consistency in BLS Registration Parameters.The recent changes remove the extraneous "pubkeyRegistrationMessageHash" parameter and rename "pubkeyRegistrationSignature" to "pubKeyRegistrationSignature" to align with the updated interface. This adjustment looks correct—just verify that all calling contracts and related unit tests (e.g., in x/avs/keeper/avs_test.go) reflect these naming changes properly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/avs/keeper/keeper.go (1)
289-319: LGTM! BLS signature validation logic is robust and secure.The implementation effectively:
- Prevents replay attacks by including chain ID and operator address in the message
- Prevents rogue key attacks by validating BLS signatures
- Prevents key reuse by checking for duplicates
Consider adding a constant for the error message format string to improve maintainability.
Apply this diff to extract the error message format string:
+const ( + ErrOperatorFormat = "the operator is :%s" +) func (k Keeper) RegisterBLSPublicKey(ctx sdk.Context, params *types.BlsParams) error { // ... - return errorsmod.Wrap(types.ErrSigNotMatchPubKey, fmt.Sprintf("the operator is :%s", params.OperatorAddress)) + return errorsmod.Wrap(types.ErrSigNotMatchPubKey, fmt.Sprintf(ErrOperatorFormat, params.OperatorAddress)) // ... - return errorsmod.Wrap(types.ErrAlreadyExists, fmt.Sprintf("the operator is :%s", params.OperatorAddress)) + return errorsmod.Wrap(types.ErrAlreadyExists, fmt.Sprintf(ErrOperatorFormat, params.OperatorAddress)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
x/avs/keeper/avs_test.go(6 hunks)x/avs/keeper/keeper.go(1 hunks)x/avs/types/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/avs/types/types.go
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-unit-cover
- GitHub Check: test-unit-e2e
- GitHub Check: goreleaser
- GitHub Check: docker-localnet-build
- GitHub Check: docker-build
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
x/avs/keeper/avs_test.go (4)
217-233: LGTM! Test case for successful registration is well-structured.The test case thoroughly validates the BLS public key registration flow by:
- Generating a random BLS key pair
- Formatting the message with chain ID and operator address
- Signing the message with the private key
- Verifying the signature
235-251: LGTM! Test case for wrong chain ID is comprehensive.The test case effectively validates that the signature verification fails when using an incorrect chain ID, preventing replay attacks across different chains.
253-270: LGTM! Test case for wrong operator address is thorough.The test case effectively validates that the signature verification fails when using a mismatched operator address, preventing impersonation attacks.
272-289: LGTM! Test case for mismatched BLS key is well-designed.The test case effectively validates that the signature verification fails when using a mismatched BLS key pair, preventing rogue key attacks.
MaxMustermann2
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/avs/keeper/avs_test.go (1)
206-307: Add test cases for additional edge cases.The test suite is well-structured using table-driven tests. Consider adding the following test cases to improve coverage:
- Invalid public key format
- Invalid signature format
- Empty operator address
testCases := []testCase{ + { + name: "invalid public key format", + setupParams: func() *types.BlsParams { + operatorAddress := sdk.AccAddress(utiltx.GenerateAddress().Bytes()) + return &types.BlsParams{ + OperatorAddress: operatorAddress, + AvsAddress: testutiltx.GenerateAddress(), + PubKey: []byte("invalid"), + } + }, + errorContains: "invalid public key format", + }, + { + name: "invalid signature format", + setupParams: func() *types.BlsParams { + privateKey, err := blst.RandKey() + suite.NoError(err) + operatorAddress := sdk.AccAddress(utiltx.GenerateAddress().Bytes()) + return &types.BlsParams{ + OperatorAddress: operatorAddress, + AvsAddress: testutiltx.GenerateAddress(), + PubKey: privateKey.PublicKey().Marshal(), + PubKeyRegistrationSignature: []byte("invalid"), + } + }, + errorContains: "invalid signature format", + }, + { + name: "empty operator address", + setupParams: func() *types.BlsParams { + privateKey, err := blst.RandKey() + suite.NoError(err) + return &types.BlsParams{ + AvsAddress: testutiltx.GenerateAddress(), + PubKey: privateKey.PublicKey().Marshal(), + PubKeyRegistrationSignature: []byte{}, + } + }, + errorContains: "empty operator address", + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/avs/keeper/avs_test.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-unit-e2e
- GitHub Check: test-unit-cover
- GitHub Check: goreleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
x/avs/keeper/avs_test.go (2)
9-9: Update import path to remove organization prefix.Remove the
ExocoreNetworkprefix from the import path to maintain consistency.- testutiltx "github.com/ExocoreNetwork/exocore/testutil/tx" + testutiltx "github.com/exocore/testutil/tx"
82-155: LGTM! Improved error handling and test robustness.The changes enhance error handling with proper nil checks and explicit action assignments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/avs/keeper/avs_test.go (1)
206-402: Enhance test coverage for BLS key registration.The test cases are comprehensive and well-structured, covering key scenarios like successful registration, key reuse, and invalid signatures. Consider adding these additional test cases to further improve coverage:
- Test with empty AVS address
- Test with malformed BLS public key (not just invalid format)
- Test concurrent registrations
- Test with maximum/minimum sized inputs
Here's a template for additional test cases:
testCases := []testCase{ + { + name: "empty avs address", + setupParams: func() *types.BlsParams { + privateKey, err := blst.RandKey() + suite.NoError(err) + operatorAddress := sdk.AccAddress(utiltx.GenerateAddress().Bytes()) + msg := fmt.Sprintf(types.BLSMessageToSign, types.ChainIDWithoutRevision(suite.Ctx.ChainID()), operatorAddress.String()) + hashedMsg := crypto.Keccak256Hash([]byte(msg)) + sig := privateKey.Sign(hashedMsg.Bytes()) + + return &types.BlsParams{ + OperatorAddress: operatorAddress, + AvsAddress: common.Address{}, + PubKey: privateKey.PublicKey().Marshal(), + PubKeyRegistrationSignature: sig.Marshal(), + } + }, + errorContains: "empty avs address", + },x/avs/keeper/keeper.go (1)
300-303: Enhance error handling specificity.The current error handling combines validation errors under
ErrSigNotMatchPubKey. Consider creating specific error types for different validation failures:
- Invalid signature format
- Invalid public key format
- Signature verification failure
Example implementation:
valid, err := blst.VerifySignature(sig, hashedMsg, pubKey) if err != nil || !valid { - return errorsmod.Wrap(types.ErrSigNotMatchPubKey, fmt.Sprintf("the operator is %s", params.OperatorAddress)) + if err != nil { + return errorsmod.Wrap(types.ErrInvalidSignatureFormat, fmt.Sprintf("invalid signature format for operator %s: %v", params.OperatorAddress, err)) + } + return errorsmod.Wrap(types.ErrSignatureVerificationFailed, fmt.Sprintf("signature verification failed for operator %s", params.OperatorAddress)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/avs/keeper/avs_test.go(6 hunks)x/avs/keeper/keeper.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-unit-e2e
- GitHub Check: test-unit-cover
- GitHub Check: goreleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
x/avs/keeper/avs_test.go (1)
9-9: Update import path prefix.The import path contains "ExocoreNetwork/exocore". Based on previous feedback, the "ExoCore-" prefix should be dropped.
x/avs/keeper/keeper.go (1)
291-297: Well-implemented replay attack prevention!The changes effectively prevent replay attacks by:
- Using a templated message format
- Including chain-id and operator-address in the message
- Using Keccak256 for message hashing
This ensures that signatures are unique to this registration function and cannot be reused across different chains or operators.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
x/avs/keeper/avs_test.go (1)
214-407: Consider adding more edge cases to strengthen test coverage.While the current test cases are comprehensive, consider adding these scenarios:
- Zero-length public key
- Zero-length signature
- Invalid operator address format
- Maximum length values for inputs
Also, the test cases for reusing BLS keys (lines 234-293) effectively validate the prevention of key reuse, which is crucial for security. Good job on including these cases!
x/avs/keeper/keeper.go (2)
300-306: Enhance error messages with more specific details.While the error handling is good, consider making the error messages more specific:
- return errorsmod.Wrap(types.ErrParsePubKey, fmt.Sprintf("the operator is %s", params.OperatorAddress)) + return errorsmod.Wrap(types.ErrParsePubKey, fmt.Sprintf("failed to parse BLS public key for operator %s: %v", params.OperatorAddress, err)) - return errorsmod.Wrap(types.ErrSigNotMatchPubKey, fmt.Sprintf("the operator is %s", params.OperatorAddress)) + return errorsmod.Wrap(types.ErrSigNotMatchPubKey, fmt.Sprintf("BLS signature verification failed for operator %s", params.OperatorAddress))
319-329: Add security rationale in comments.The code effectively prevents BLS key reuse, but consider adding a comment explaining the security implications:
- // verify that the BLS key is not already in use by - // (1) even the same operator on a different AVS - // (2) different operators on the same AVS - // this design decision is made to ensure that the same BLS key - // is not reused across different AVSs by the same operator + // Prevent BLS key reuse to mitigate several security risks: + // 1. Rogue key attacks where an attacker claims ownership of another's key + // 2. Cross-AVS replay attacks where signatures from one AVS could be reused in another + // 3. Key sharing between operators which could lead to collusion + // Therefore, we ensure each BLS key is unique across: + // - Different AVSs (even for the same operator) + // - Different operators (even within the same AVS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
precompiles/avs/tx.go(1 hunks)x/avs/keeper/avs_test.go(6 hunks)x/avs/keeper/keeper.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- precompiles/avs/tx.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-unit-cover
- GitHub Check: test-unit-e2e
- GitHub Check: goreleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
x/avs/keeper/avs_test.go (2)
9-9: Update import path to drop "ExoCore-" prefix.The import path contains "ExocoreNetwork/exocore" which should be updated based on previous feedback.
207-212: Well-structured test function!The test function follows best practices:
- Uses table-driven tests for better organization
- Each test case has a descriptive name
- Setup is isolated per test case
- Clear error expectations
x/avs/keeper/keeper.go (1)
291-297: Excellent security measures for preventing replay attacks!The implementation effectively prevents replay attacks by:
- Including chain ID in the message
- Including operator address in the message
- Using a templated message format to ensure consistency
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/avs/keeper/avs_test.go (1)
207-422: Excellent test coverage for BLS key registration!The test cases thoroughly cover important security scenarios including:
- Successful registration
- Replay attack prevention
- Chain ID validation
- Signature verification
- Public key format validation
Consider adding these edge cases:
- Empty AVS address validation
- Nil public key and signature handling
- Maximum length validation for public key and signature
{ + name: "empty avs address", + setupParams: func() *types.BlsParams { + privateKey, err := blst.RandKey() + suite.NoError(err) + operatorAddress := sdk.AccAddress(utiltx.GenerateAddress().Bytes()) + msg := fmt.Sprintf(types.BLSMessageToSign, types.ChainIDWithoutRevision(suite.Ctx.ChainID()), operatorAddress.String()) + hashedMsg := crypto.Keccak256Hash([]byte(msg)) + sig := privateKey.Sign(hashedMsg.Bytes()) + return &types.BlsParams{ + OperatorAddress: operatorAddress, + AvsAddress: common.Address{}, + PubKey: privateKey.PublicKey().Marshal(), + PubKeyRegistrationSignature: sig.Marshal(), + } + }, + errorContains: types.ErrInvalidAddr.Error(), + }, + { + name: "nil public key and signature", + setupParams: func() *types.BlsParams { + operatorAddress := sdk.AccAddress(utiltx.GenerateAddress().Bytes()) + return &types.BlsParams{ + OperatorAddress: operatorAddress, + AvsAddress: testutiltx.GenerateAddress(), + PubKey: nil, + PubKeyRegistrationSignature: nil, + } + }, + errorContains: types.ErrParsePubKey.Error(), + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/avs/keeper/avs_test.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-unit-e2e
- GitHub Check: test-unit-cover
- GitHub Check: goreleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
x/avs/keeper/avs_test.go (2)
9-9: Update import path to remove "ExocoreNetwork" prefix.Based on previous review comments, the "ExocoreNetwork" prefix should be dropped from the import path.
- testutiltx "github.com/ExocoreNetwork/exocore/testutil/tx" + testutiltx "github.com/exocore/testutil/tx"
86-86: LGTM! Action field assignments are consistent.The changes to the
Actionfield assignments in the test cases are consistent and maintain the test coverage for AVS registration and deregistration flows.Also applies to: 118-118, 131-131, 147-147
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
x/avs/keeper/avs_test.go (1)
227-442: Well-structured test cases with comprehensive coverage.The test cases thoroughly cover various scenarios including successful registration, key reuse, invalid signatures, and format validation. However, there are a few potential improvements:
- Consider adding a test case for empty AVS address
- The test message format could be moved to a constant for reusability
Consider adding this test case:
+ { + name: "empty avs address", + setupParams: func() *types.BlsParams { + privateKey, err := blst.RandKey() + suite.NoError(err) + operatorAddress := sdk.AccAddress(utiltx.GenerateAddress().Bytes()) + msg := fmt.Sprintf(types.BLSMessageToSign, types.ChainIDWithoutRevision(suite.Ctx.ChainID()), operatorAddress.String()) + hashedMsg := crypto.Keccak256Hash([]byte(msg)) + sig := privateKey.Sign(hashedMsg.Bytes()) + + return &types.BlsParams{ + OperatorAddress: operatorAddress, + AvsAddress: common.Address{}, + PubKey: privateKey.PublicKey().Marshal(), + PubKeyRegistrationSignature: sig.Marshal(), + } + }, + errorContains: types.ErrInvalidAddr.Error(), + },x/avs/keeper/keeper.go (2)
300-306: Improve error handling consistency.While the error handling is generally good, the error messages could be more consistent. Consider using a consistent format for all error messages.
Apply this diff to make error messages more consistent:
- return errorsmod.Wrap(types.ErrParsePubKey, fmt.Sprintf("the operator is %s", params.OperatorAddress)) + return errorsmod.Wrap(types.ErrParsePubKey, fmt.Sprintf("failed to parse public key for operator: %s", params.OperatorAddress)) - return errorsmod.Wrap(types.ErrSigNotMatchPubKey, fmt.Sprintf("the operator is %s", params.OperatorAddress)) + return errorsmod.Wrap(types.ErrSigNotMatchPubKey, fmt.Sprintf("signature verification failed for operator: %s", params.OperatorAddress))
319-330: Strong security measures against key reuse.The implementation effectively prevents BLS key reuse across different AVSs and operators. Consider adding validation for the AVS address to ensure it's not empty.
Add this validation at the start of the function:
func (k Keeper) RegisterBLSPublicKey(ctx sdk.Context, params *types.BlsParams) error { + if bytes.Equal(params.AvsAddress.Bytes(), common.Address{}.Bytes()) { + return errorsmod.Wrap(types.ErrInvalidAddr, "AVS address cannot be empty") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
precompiles/avs/IAVSManager.sol(1 hunks)precompiles/avs/tx.go(1 hunks)x/avs/keeper/avs_test.go(6 hunks)x/avs/keeper/keeper.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- precompiles/avs/IAVSManager.sol
- precompiles/avs/tx.go
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-unit-e2e
- GitHub Check: test-unit-cover
- GitHub Check: docker-build
- GitHub Check: docker-localnet-build
- GitHub Check: Analyze (go)
- GitHub Check: goreleaser
🔇 Additional comments (2)
x/avs/keeper/avs_test.go (1)
100-100: LGTM! Action type references updated correctly.The changes correctly update the action type references to use the types package.
Also applies to: 136-136, 149-149
x/avs/keeper/keeper.go (1)
291-297: Good security improvement for preventing replay attacks.The templated message format effectively prevents replay attacks by including chain-id and operator-address. The comments clearly explain the security benefits.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/avs/keeper/avs_test.go (1)
222-437: Well-structured test suite with comprehensive coverage!The test suite effectively covers various security scenarios including replay attacks, signature validation, and input validation. This aligns well with the PR's security objectives.
Consider these improvements to enhance maintainability:
- Extract repeated test values into constants
- Group related test cases using subtests
- Add verification of stored data in the successful case
Apply this diff to improve the test structure:
func (suite *AVSTestSuite) TestRegisterBLSPublicKey() { + const ( + invalidChainID = "onemorechain_211" + invalidBytes = "invalid" + ) + type testCase struct { name string setupParams func() *types.BlsParams errorContains string + verifyState func(*types.BlsParams) } testCases := []testCase{ { name: "successful registration", setupParams: func() *types.BlsParams { // ... existing setup code ... }, + verifyState: func(params *types.BlsParams) { + // Verify the key was stored correctly + storedKey, err := suite.App.AVSManagerKeeper.GetBLSKey( + suite.Ctx, + params.OperatorAddress, + params.AvsAddress, + ) + suite.NoError(err) + suite.Equal(params.PubKey, storedKey) + }, }, // Group related test cases + { + name: "key reuse scenarios", + subtests: []testCase{ + // Move key reuse test cases here + }, + }, // ... rest of the test cases ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
precompiles/avs/query_test.go(1 hunks)x/avs/keeper/avs_test.go(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- precompiles/avs/query_test.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-unit-e2e
- GitHub Check: test-unit-cover
- GitHub Check: goreleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
x/avs/keeper/avs_test.go (1)
3-26: Clean up duplicate imports.There are duplicate imports that need to be cleaned up:
avstypesis imported buttypesis already importedutiltxfromevmosis imported buttestutiltxis already imported
Description
refactor: improve bls signature validation logic
test: add unit tests for BLS signature functionality
update the pre-compiled bls key registration interface parameters
Summary by CodeRabbit