refactor: separate validator from rest of server code #806
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “graphite/merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
WalkthroughThe changes in this pull request primarily focus on refactoring the transaction validation logic within the server's handling of transactions. The Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
🪧 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range comments (4)
cardinal/server/option.go (1)
Line range hint
1-45: Consider adding configuration documentationThe consolidation of configuration fields is a good architectural improvement. Consider adding a documented
Configstruct type to make the configuration structure more explicit and self-documenting.Example addition:
// Config holds all server configuration options type Config struct { port string isSwaggerDisabled bool isSignatureValidationDisabled bool messageExpirationSeconds int messageHashCacheSizeKB int // ... other config fields }cardinal/server/server.go (1)
Line range hint
161-161: Replaces.verifywiths.validatorinhandler.PostTransactionThe
s.verifyfield no longer exists in theServerstruct after the refactoring. It should be updated tos.validatorto reflect the changes and ensure the transaction handler uses the correct validator.Apply this diff to fix the issue:
-tx.Post("/:group/:name", handler.PostTransaction(world, msgIndex, s.verify)) +tx.Post("/:group/:name", handler.PostTransaction(world, msgIndex, s.validator))cardinal/server/handler/tx.go (2)
Line range hint
174-182: Fix undefined variable 'verify' to 'validator' in conditional statementIn the
extractTxfunction, the variableverifyis not defined in this scope. It appears you intended to usevalidatorinstead ofverify.Apply the following changes to correct the variable name and references:
- if !verify.IsDisabled { + if !validator.IsDisabled { // we are doing signature verification, so use sign's Unmarshal which does extra checks tx, err = sign.UnmarshalTransaction(ctx.Body()) } else { // we aren't doing signature verification, so just use the generic body parser which is more forgiving tx = new(sign.Transaction) err = ctx.BodyParser(tx) } if err != nil { log.Errorf("body parse failed: %v", err) return nil, fiber.NewError(fiber.StatusBadRequest, "Bad Request - unparseable body") } - if !verify.IsDisabled { + if !validator.IsDisabled { txEarliestValidTimestamp := sign.TimestampAt( - time.Now().Add(-(time.Duration(verify.MessageExpirationSeconds) * time.Second))) + time.Now().Add(-(time.Duration(validator.MessageExpirationSeconds) * time.Second)))🧰 Tools
🪛 GitHub Check: Lint (go)
[failure] 9-9:
"github.com/ethereum/go-ethereum/common" imported and not used
Line range hint
184-192: Remove obsolete cache logic and undefined function 'isHashInCache'The cache-related logic for replay protection has been removed, but the code still references
isHashInCacheandvalidator.Cache. SinceisHashInCacheis undefined and the cache logic is no longer utilized, these lines should be removed to prevent compilation errors.Apply the following diff to eliminate the obsolete cache check:
// before we even create the hash or validate the signature, check to see if the message has expired if tx.Timestamp < txEarliestValidTimestamp { log.Errorf("message older than %d seconds. Got timestamp: %d, current timestamp: %d ", - verify.MessageExpirationSeconds, tx.Timestamp, sign.TimestampNow()) + validator.MessageExpirationSeconds, tx.Timestamp, sign.TimestampNow()) return nil, fiber.NewError(fiber.StatusRequestTimeout, "Request Timeout - signature too old") } - // check for duplicate message via hash cache - if found, err := isHashInCache(tx.Hash, validator.Cache); err != nil { - log.Errorf("unexpected cache error %v. message %s ignored", err, tx.Hash.String()) - return nil, fiber.NewError(fiber.StatusInternalServerError, "Internal Server Error - cache failed") - } else if found { - // if found in the cache, the message hash has already been used, so reject it - log.Errorf("message %s already handled", tx.Hash.String()) - return nil, fiber.NewError(fiber.StatusForbidden, "Forbidden - duplicate message") - } // at this point we know that the generated hash is not in the cache, so this message is not a replay }🧰 Tools
🪛 GitHub Check: Lint (go)
[failure] 9-9:
"github.com/ethereum/go-ethereum/common" imported and not used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (9)
cardinal/server/handler/tx.go(7 hunks)cardinal/server/option.go(3 hunks)cardinal/server/server.go(3 hunks)cardinal/server/types/provider.go(1 hunks)cardinal/server/validator/validator.go(1 hunks)cardinal/server/validator/validator_test.go(1 hunks)cardinal/world_persona.go(1 hunks)sign/sign.go(1 hunks)sign/sign_test.go(1 hunks)
🧰 Additional context used
📓 Learnings (2)
cardinal/server/validator/validator.go (1)
Learnt from: ezavada
PR: Argus-Labs/world-engine#804
File: cardinal/server/validator/validator.go:174-176
Timestamp: 2024-11-01T14:41:04.393Z
Learning: In `validator.go`, ensure that the `ValidateTransactionTTL` function is restricted to validating the TTL of the message only. Do not include other validations, such as namespace validation, within this function.
sign/sign_test.go (1)
Learnt from: ezavada
PR: Argus-Labs/world-engine#801
File: sign/sign_test.go:22-24
Timestamp: 2024-10-21T15:26:02.039Z
Learning: When tests in `sign/sign_test.go` already assert that a timestamp is within a range between a stored time and `time.Now().UnixMicro()`, suggesting to add additional tolerance may not be necessary.
🔇 Additional comments (17)
cardinal/server/types/provider.go (2)
6-6: LGTM: Clean import addition
The addition of the validator package import aligns with the PR's objective of separating validator code.
12-12: LGTM: Good use of interface embedding
The embedding of SignerAddressProvider follows Go best practices and promotes better separation of concerns. Let's verify the implementation completeness across the codebase.
✅ Verification successful
Based on the results, I can now generate the final response since I have found the implementations of GetSignerAddressForPersonaTag:
Verified: Interface implementation is complete and correct
The SignerAddressProvider interface is properly implemented in both required locations:
- Main implementation in
Worldstruct (cardinal/world_persona.go) which delegates toGetSignerForPersonaTag - Test implementation in
ProviderFixture(cardinal/server/validator/validator_test.go) for validation testing
Both implementations follow the correct method signature and provide the expected functionality for persona tag validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all implementations of ProviderWorld interface
# also implement SignerAddressProvider methods
# First, find the SignerAddressProvider interface definition
echo "Checking SignerAddressProvider interface definition:"
ast-grep --pattern 'type SignerAddressProvider interface {
$$$
}'
# Then, find all structs that implement ProviderWorld
echo -e "\nChecking ProviderWorld implementations:"
ast-grep --pattern 'type $_ struct {
$$$
}'
# Finally, search for GetSignerAddressForPersonaTag implementations
echo -e "\nChecking GetSignerAddressForPersonaTag implementations:"
ast-grep --pattern 'func ($_ $_) GetSignerAddressForPersonaTag($_) ($_, $_) {
$$$
}'
Length of output: 138062
cardinal/server/option.go (3)
22-22: LGTM! Field rename improves clarity
The change from verify.IsDisabled to config.isSignatureValidationDisabled is more explicit and aligns well with the refactoring goal of centralizing configuration.
45-45: Verify hash cache initialization in validator
The configuration move looks good and the renamed field better indicates its purpose. Since this affects replay protection, let's verify the cache initialization.
#!/bin/bash
# Description: Verify proper hash cache initialization and usage
# Expected: Find cache initialization code and size configuration usage
# Search for cache initialization
ast-grep --pattern 'type $_ struct {
$$$
cache $$$
$$$
}'
# Search for cache size configuration usage
rg -A 3 "messageHashCacheSizeKB" --type go36-36: Verify message expiration handling in validator package
The configuration move looks good, but let's verify that the new validator package correctly accesses this configuration for expiration checks.
✅ Verification successful
Let me gather more information about how the message expiration is used in validation.
Configuration change is properly handled in validator package
The verification shows that the message expiration configuration is correctly used in the validator package:
- The validator correctly receives and stores the expiration seconds in
MessageExpirationSecondsfield - The expiration check is properly implemented in validation logic using this configuration
- The cache retention also correctly uses this value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the message expiration config is properly used in the validator
# Expected: Find references to messageExpirationSeconds in validator logic
# Search for message expiration usage in validator package
rg -l "messageExpirationSeconds" --type go
# Search for time-based validation logic in validator
ast-grep --pattern 'func $_($$) {
$$$
time.$$$
$$$
}'
Length of output: 1470
Script:
#!/bin/bash
# Search for validation logic related to message expiration
rg -A 5 "MessageExpirationSeconds" --type go
# Search for time-based validation in validator package
rg -A 5 "time\." cardinal/server/validator/
Length of output: 5839
cardinal/world_persona.go (2)
15-17: LGTM: Clean integration with existing code.
The implementation correctly leverages the existing GetSignerForPersonaTag method while maintaining proper error handling and preserving the original behavior.
14-18: Verify SignerAddressProvider implementation completeness.
Let's verify this is the only implementation of SignerAddressProvider and check its usage.
✅ Verification successful
Let me generate additional verification scripts to check the interface definition and its complete usage pattern.
Implementation verified and correctly follows interface contract
The verification confirms:
- The interface
SignerAddressProvideris properly defined incardinal/server/validator/validator.go - The
Worldstruct correctly implements the interface with matching method signature - There are two implementations:
- Main implementation in
Worldstruct - Test implementation in
ProviderFixturefor testing purposes
- Main implementation in
- The interface is used appropriately in the validator package for signature verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other SignerAddressProvider implementations
echo "Searching for SignerAddressProvider interface implementations..."
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) GetSignerAddressForPersonaTag($_) ($_, $_) {
$$$
}'
# Search for method usage
echo "Searching for GetSignerAddressForPersonaTag usage..."
rg "GetSignerAddressForPersonaTag" -A 3
Length of output: 2003
Script:
#!/bin/bash
# Search for SignerAddressProvider interface definition
echo "Searching for SignerAddressProvider interface definition..."
ast-grep --pattern 'type SignerAddressProvider interface {
$$$
}'
# Search for signerAddressProvider field usage
echo "Searching for signerAddressProvider field usage..."
rg "signerAddressProvider" -A 2
# Search for any other potential implementations
echo "Searching for potential struct types implementing the interface..."
rg "SignerAddressProvider" --type go
Length of output: 1887
sign/sign.go (1)
50-64: Verify the usage of timestamp functions across the codebase.
Let's ensure all callers of these functions handle the timestamps correctly.
✅ Verification successful
Based on the search results, I can now provide a final response as the verification is complete:
Timestamp functions are used consistently and safely across the codebase
The verification shows:
- All timestamp functions are used correctly in their respective contexts
- Proper validation of timestamps is implemented in validators and handlers
- Consistent millisecond precision is maintained throughout
- No breaking changes detected as all callers handle timestamps appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of the timestamp functions to verify proper handling
echo "Searching for TimestampNow usage..."
rg "TimestampNow\(\)" -A 2
echo "Searching for TimestampAt usage..."
rg "TimestampAt\(" -A 2
echo "Searching for direct Timestamp usage..."
rg "Timestamp\(" -A 2
# Look for potential timestamp comparisons that might need validation
echo "Checking for timestamp comparisons..."
rg "UnixMilli|\.Timestamp" -A 2
Length of output: 17767
sign/sign_test.go (1)
112-116: LGTM: Basic timestamp validation is correct.
The test correctly verifies that transaction timestamps are generated within an acceptable range of the current time.
cardinal/server/server.go (6)
17-17: Importing the validator package
The addition of the validator package import is necessary for the implementation of the SignatureValidator and aligns with the refactoring objectives.
27-27: Define defaultHashCacheSizeKB constant appropriately
Setting defaultHashCacheSizeKB to 1024 KB (1MB) provides a reasonable default size for the hash cache.
31-35: Enhance configuration flexibility with additional fields
Adding new fields to the config struct allows for greater customization and better management of server settings such as port, Swagger availability, signature validation, message expiration, and hash cache size.
39-41: Include validator in the Server struct
Incorporating the validator field into the Server struct encapsulates the signature validation logic and improves the separation of concerns.
59-63: Initialize configuration with default values
Initializing config with default values ensures consistent server behavior when specific options are not provided.
70-77: Instantiate SignatureValidator with current configuration
Creating the SignatureValidator using the existing configuration parameters helps maintain consistency and centralizes signature validation logic.
cardinal/server/handler/tx.go (1)
Line range hint 157-170: Ensure consistent usage of 'validator' in 'extractTx' function
While you've updated the extractTx function to accept validator, ensure that all references within the function consistently use validator instead of any outdated variables.
Please review the function to confirm that all instances of the old variable (e.g., verify) have been replaced with validator.
cardinal/server/validator/validator_test.go (1)
103-398: Comprehensive test coverage
The test suite provides extensive coverage of the SignatureValidator functionality, testing various scenarios including signature verification enabled and disabled, invalid signatures, missing persona tags, altered transactions, and replay attacks. This thorough testing ensures the robustness and reliability of the validation logic.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #806 +/- ##
==========================================
+ Coverage 57.95% 58.18% +0.23%
==========================================
Files 143 144 +1
Lines 9188 9237 +49
==========================================
+ Hits 5325 5375 +50
- Misses 3414 3415 +1
+ Partials 449 447 -2
|
ezavada
force-pushed
the
edz/world-1237-new
branch
from
6dbf173 to
5c9d0f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (9)
cardinal/server/handler/tx.go(6 hunks)cardinal/server/option.go(3 hunks)cardinal/server/server.go(4 hunks)cardinal/server/types/provider.go(1 hunks)cardinal/server/validator/validator.go(1 hunks)cardinal/server/validator/validator_test.go(1 hunks)cardinal/world_persona.go(1 hunks)sign/sign.go(1 hunks)sign/sign_test.go(1 hunks)
🧰 Additional context used
📓 Learnings (2)
cardinal/server/validator/validator.go (1)
Learnt from: ezavada
PR: Argus-Labs/world-engine#804
File: cardinal/server/validator/validator.go:174-176
Timestamp: 2024-11-01T14:41:04.393Z
Learning: In `validator.go`, ensure that the `ValidateTransactionTTL` function is restricted to validating the TTL of the message only. Do not include other validations, such as namespace validation, within this function.
sign/sign_test.go (1)
Learnt from: ezavada
PR: Argus-Labs/world-engine#801
File: sign/sign_test.go:22-24
Timestamp: 2024-10-21T15:26:02.039Z
Learning: When tests in `sign/sign_test.go` already assert that a timestamp is within a range between a stored time and `time.Now().UnixMicro()`, suggesting to add additional tolerance may not be necessary.
🔇 Additional comments (22)
cardinal/server/types/provider.go (2)
6-6: LGTM: Import addition aligns with refactoring goals.
The new validator package import supports the separation of validation logic into its own package.
12-12: Verify all ProviderWorld implementations are updated.
The embedding of validator.SignerAddressProvider aligns well with the refactoring goals. However, this is a breaking change that requires all implementations of ProviderWorld to implement the SignerAddressProvider interface methods.
Let's verify all implementations are properly updated:
✅ Verification successful
Let me gather more information about the ProviderWorld interface and its implementations.
Let me search for the World struct implementation since it's likely the main implementer of ProviderWorld.
Based on the evidence gathered, I can now provide a final response:
Implementation is properly updated and consistent
The World struct correctly implements the SignerAddressProvider interface through its GetSignerAddressForPersonaTag method, which internally calls GetSignerForPersonaTag with appropriate tick handling. The implementation is consistent with the interface embedding in ProviderWorld.
Key observations:
GetSignerAddressForPersonaTagis implemented incardinal/world_persona.go- The implementation preserves existing behavior by using tick 0 for backward compatibility
- The method properly delegates to
GetSignerForPersonaTagwhich contains the core logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of ProviderWorld to ensure they're updated
# with SignerAddressProvider methods
# Search for struct types that might implement ProviderWorld
ast-grep --pattern 'type $_ struct {
$$$
}'
# Search for potential interface implementation methods
rg -A 2 "func \(.*\) GetSignerAddressForPersonaTag"
Length of output: 136692
Script:
#!/bin/bash
# Find the ProviderWorld interface definition and its implementations
ast-grep --pattern 'type ProviderWorld interface {
$$$
}'
# Search for potential implementations
rg -A 5 'type \w+ struct.*{.*}.*\n.*implements.*ProviderWorld'
Length of output: 1619
Script:
#!/bin/bash
# Search for World struct implementation of GetSignerAddressForPersonaTag
rg -A 5 'func \(w \*World\) GetSignerAddressForPersonaTag'
# Search for World struct implementation of GetSignerForPersonaTag
rg -A 5 'func \(w \*World\) GetSignerForPersonaTag'
# Look for any other implementations
rg -A 5 'implements.*ProviderWorld'
Length of output: 1061
cardinal/server/option.go (2)
22-22: LGTM! Clear and consistent configuration field name.
The renamed configuration field isSignatureValidationDisabled is more explicit and follows Go naming conventions.
Line range hint 22-45: Verify complete migration of verification-related configurations.
The consolidation of configuration fields from verify to config looks good. Let's ensure all verification-related configurations have been migrated consistently across the codebase.
✅ Verification successful
Let me gather more specific information about the configuration usage.
Based on the results, I can now generate the final response since I have sufficient evidence about the configuration migration.
Configuration migration is properly implemented
The verification-related configurations have been correctly migrated to the config structure and are consistently used throughout the codebase:
- All configuration fields (
isSignatureValidationDisabled,messageExpirationSeconds,messageHashCacheSizeKB) are properly defined in the server's config structure - The configuration values are correctly passed to the
SignatureValidatorduring initialization - No lingering references to the old verify structure were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old verify structure
rg -l "verify\." --type go
# Search for all config field usages to ensure consistent naming
ast-grep --pattern 'config.$_'
Length of output: 164
Script:
#!/bin/bash
# Search for verify-related configuration in the codebase
rg -A 3 "isSignatureValidation|messageExpiration|hashCache" --type go
# Look for any Server struct definitions to understand the configuration structure
ast-grep --pattern 'type Server struct {
$$$
}'
# Search for any verify package imports
rg -l "\".*\/verify\"" --type go
Length of output: 2336
cardinal/world_persona.go (1)
14-18: LGTM with verification needed.
The implementation correctly delegates to GetSignerForPersonaTag while preserving existing behavior. However, let's verify the error handling aligns with the validator's expectations.
✅ Verification successful
Error handling in GetSignerAddressForPersonaTag is correctly implemented
The implementation aligns perfectly with the validator's expectations:
- The interface in
cardinal/server/validator/validator.gorequires(string, error)return values, which matches the implementation - The validator properly handles errors from the provider by wrapping them in a validation error with appropriate HTTP status code (401 Unauthorized)
- The test fixture demonstrates the expected error handling pattern, particularly for the "no signer" scenario
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling alignment between World and SignatureValidator
# Test 1: Check how SignatureValidator handles errors from SignerAddressProvider
ast-grep --pattern 'type SignerAddressProvider interface {
$$$
GetSignerAddressForPersonaTag($$$) (string, error)
$$$
}'
# Test 2: Check error handling in SignatureValidator when using the provider
rg -A 5 'GetSignerAddressForPersonaTag' 'server/validator/'
Length of output: 244
Script:
#!/bin/bash
# Let's try searching in the entire codebase for the interface and its usage
# Search for interface definition
ast-grep --pattern 'type SignerAddressProvider interface {
$$$
}'
# Search for validator package and interface usage
rg -l "SignerAddressProvider"
# Search for any validation-related code that uses GetSignerAddressForPersonaTag
rg -A 5 "GetSignerAddressForPersonaTag"
Length of output: 2727
sign/sign.go (3)
50-53: Improve function documentation to follow Go standards.
The documentation should follow Go standards and clearly specify the timestamp format.
55-58: Improve function documentation to follow Go standards.
The documentation should follow Go standards and clearly specify the timestamp format.
60-64: Add input validation for secure timestamp handling.
Given that this function is used by the new validator package for transaction TTL checks, proper input validation is crucial for secure transaction validation.
Let's verify the validator's usage of this function:
#!/bin/bash
# Search for usage of Timestamp function in validator package
rg -A 5 "Timestamp\(" server/validator/sign/sign_test.go (1)
117-125: Consider improving test reliability by mocking time.
The test uses time.Sleep and makes millisecond-level timing assumptions which could lead to flaky tests, especially in CI environments with varying load conditions.
This was previously suggested in an earlier review. The recommendation to use a clock interface for deterministic testing still applies here.
cardinal/server/server.go (8)
17-17: Necessary import of the validator package added
The import statement correctly adds the validator package, which is required for the SignatureValidator implementation.
27-27: Default hash cache size constant defined appropriately
The defaultHashCacheSizeKB constant is set to 1024, accurately representing a default cache size of 1MB as per the comment.
31-35: Configuration struct updated with new fields for enhanced flexibility
The config struct now includes additional fields:
portisSwaggerDisabledisSignatureValidationDisabledmessageExpirationSecondsmessageHashCacheSizeKB
These fields enhance the server's configurability and allow for more granular control over its behavior.
39-41: Server struct updated with configuration and validator
The Server struct now includes:
configfield for server configurationvalidatorfield of type*validator.SignatureValidatorfor encapsulating validation logic
These additions improve the modularity and maintainability of the server code.
59-63: Default configuration values are set appropriately
The default values assigned to the config fields ensure sensible defaults:
port: defaults to"4040"isSwaggerDisabled:false(Swagger is enabled by default)isSignatureValidationDisabled:false(Signature validation is enabled by default)messageExpirationSeconds: set to10secondsmessageHashCacheSizeKB: set to1024KB (1MB)
70-77: SignatureValidator initialized correctly with configuration parameters
The SignatureValidator is instantiated using the updated configuration fields and the world object:
s.config.isSignatureValidationDisableds.config.messageExpirationSecondss.config.messageHashCacheSizeKBworld.Namespace()world(as a provider of signature addresses)
This setup centralizes the validation logic and simplifies transaction handling.
187-187: Transaction handler updated to use the new SignatureValidator
The PostTransaction handler now includes s.validator as a parameter, aligning with the refactored validation logic.
70-77:
Ensure proper error handling when initializing SignatureValidator
Currently, the NewSignatureValidator function is called without checking for errors. If NewSignatureValidator can return an error, it should be handled to prevent potential runtime issues.
Apply this diff to handle possible errors:
func New(
world servertypes.ProviderWorld,
components []types.ComponentMetadata,
messages []types.Message,
opts ...Option,
) (*Server, error) {
// Existing code...
// Initialize the SignatureValidator
- s.validator = validator.NewSignatureValidator(
+ validator, err := validator.NewSignatureValidator(
s.config.isSignatureValidationDisabled,
s.config.messageExpirationSeconds,
s.config.messageHashCacheSizeKB,
world.Namespace(),
world, // world is a provider of signature addresses
)
+ if err != nil {
+ return nil, eris.Wrap(err, "failed to create SignatureValidator")
+ }
+ s.validator = validator
// Existing code...
}Please ensure that NewSignatureValidator returns an error if initialization fails.
cardinal/server/handler/tx.go (3)
138-140: 🧹 Nitpick (assertive)
Fix typo in comment
There's a typo in the comment: "with" should be "which".
Apply this diff to correct the typo:
} else {
- // we aren't doing signature verification, so just use the generic body parser with is more forgiving
+ // we aren't doing signature verification, so just use the generic body parser which is more forgiving
tx = new(sign.Transaction)Likely invalid or redundant comment.
5-5:
Import the standard 'log' package instead of 'github.com/gofiber/fiber/v2/log'
The github.com/gofiber/fiber/v2/log package does not exist. To use logging functions like log.Errorf, you should import the standard library's log package.
Apply this diff to fix the import:
import (
"github.com/gofiber/fiber/v2"
- "github.com/gofiber/fiber/v2/log"
+ "log"
personaMsg "pkg.world.dev/world-engine/cardinal/persona/msg"
servertypes "pkg.world.dev/world-engine/cardinal/server/types"Likely invalid or redundant comment.
75-77: Ensure ValidateTransactionSignature handles empty signerAddress correctly
When signerAddress is an empty string (for messages other than CreatePersona), confirm that validator.ValidateTransactionSignature handles this case appropriately and does not result in incorrect signature validation.
Run the following script to verify the handling of empty signerAddress:
This script searches for the implementation of ValidateTransactionSignature in the codebase. Please review the function to ensure it safely handles an empty signerAddress.
✅ Verification successful
These scripts will help us:
- Find direct references to ValidateTransactionSignature
- Locate validator-related files
- Find any signature validation related code using broader patterns
This will help us understand the complete implementation of the validator, including how it handles empty signerAddress cases.
Empty signerAddress is properly handled by the validator
The implementation in validator.go shows that empty signerAddress is handled correctly:
- When
signerAddressis empty, the validator automatically looks up the correct signer address using thesignerAddressProvider:
if signerAddress == "" {
signerAddress, err = validator.signerAddressProvider.GetSignerAddressForPersonaTag(tx.PersonaTag)
if err != nil {
return &validationError{ErrInvalidSignature, http.StatusUnauthorized,
fmt.Sprintf("could not get signer for persona %s: %v", tx.PersonaTag, err)}
}
}- The only requirement is that
PersonaTagmust not be empty, which is checked first:
if tx.PersonaTag == "" {
return &validationError{ErrNoPersonaTag, http.StatusBadRequest,
fmt.Sprintf("Missing persona tag for message %s", tx.Hash.String())}
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if ValidateTransactionSignature handles empty signerAddress.
# Test: Search for the implementation of ValidateTransactionSignature.
# Expect: It should handle empty signerAddress without errors.
ast-grep --pattern $'func ValidateTransactionSignature($_, $_) $$$_' --lang go
Length of output: 79
Script:
#!/bin/bash
# Let's try a broader search to locate the validator package and its implementation
rg -g "*.go" "ValidateTransactionSignature" -A 5
Length of output: 12935
Script:
#!/bin/bash
# Also search for any validator related files
fd validator -e go
Length of output: 103
Script:
#!/bin/bash
# Search for any signature validation related code
rg -g "*.go" "signature.*validation|validate.*signature" -i
Length of output: 6957
Script:
#!/bin/bash
# Let's examine the validator implementation more closely
cat cardinal/server/validator/validator.go
Length of output: 7122
cardinal/server/validator/validator.go (2)
89-117: Efficient Time Validation in ValidateTransactionTTL
The function correctly calculates the acceptable time range for transactions and properly handles expired and future-dated messages. This ensures that only transactions within the valid time window are processed.
119-158: Comprehensive Signature Validation in ValidateTransactionSignature
The method thoroughly validates transaction signatures, including handling cases where the signer address needs to be retrieved. It also appropriately caches valid transaction hashes to prevent replay attacks, enhancing security.
- Still need to get rid of the custom ValidationError in favor of eris.wrap
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (1)
cardinal/server/handler/tx.go (1)
Line range hint
133-148: Returnfiber.Errorfor proper HTTP status codesIn
extractTx, wrapping the error witheris.Wrapmay result in a 500 Internal Server Error when returned. To return the intended 400 Bad Request status, usefiber.NewError.Apply this diff to return a
fiber.Errorwith the correct status code:- return nil, eris.Wrap(err, "Bad Request - unparseable body") + return nil, fiber.NewError(fiber.StatusBadRequest, "Bad Request - unparseable body")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
cardinal/option.go(1 hunks)cardinal/server/handler/tx.go(8 hunks)cardinal/server/option.go(2 hunks)cardinal/server/server.go(4 hunks)cardinal/server/validator/validator.go(1 hunks)cardinal/server/validator/validator_test.go(1 hunks)cardinal/world_persona.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
cardinal/server/option.go (1)
Learnt from: smsunarto
PR: Argus-Labs/world-engine#806
File: cardinal/server/option.go:45-45
Timestamp: 2024-11-02T13:36:23.178Z
Learning: In `cardinal/server/option.go`, within the `WithHashCacheSize` function, enforce a minimum cache size and log a warning if the provided size is too small.
🔇 Additional comments (23)
cardinal/server/option.go (2)
22-22: LGTM: Config field name change is consistent.
The transition from verify.IsDisabled to config.isSignatureValidationDisabled aligns well with the refactoring objectives while maintaining clear naming.
34-36: LGTM: Type change improves parameter validation.
The change from int to uint naturally prevents negative values, which is an improvement over the previous implementation. The config field change aligns with the overall refactoring.
cardinal/world_persona.go (2)
Line range hint 1-11: Missing validator package import.
The code indicates implementation of validator.SignerAddressProvider interface but the validator package is not imported. This is a critical issue that needs to be addressed.
Since this was flagged in a previous review and is still valid, please add the missing import:
import (
"errors"
"github.com/rotisserie/eris"
"pkg.world.dev/world-engine/cardinal/filter"
"pkg.world.dev/world-engine/cardinal/persona"
"pkg.world.dev/world-engine/cardinal/persona/component"
"pkg.world.dev/world-engine/cardinal/types"
+ "pkg.world.dev/world-engine/server/validator"
)Line range hint 17-45: Verify interface contract implementation.
The method seems to implement the validator.SignerAddressProvider interface, but we should verify that it fully satisfies the interface contract from the validator package.
✅ Verification successful
Based on the output, I can now generate the final response since I have found the interface definition and can verify the implementation matches.
Interface contract is correctly implemented ✅
The GetSignerForPersonaTag method in World correctly implements the SignerAddressProvider interface from cardinal/server/validator/validator.go. The method signature matches exactly:
- Same name:
GetSignerForPersonaTag - Same parameters:
personaTag string, tick uint64 - Same return types:
(addr string, err error)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the SignerAddressProvider interface implementation
# Expected: Find the interface definition and any other implementations for comparison
# Search for the interface definition
echo "=== SignerAddressProvider Interface Definition ==="
ast-grep --pattern 'type SignerAddressProvider interface {
$$$
}'
# Search for other implementations of the interface
echo -e "\n=== Other Interface Implementations ==="
rg -p "implements.*SignerAddressProvider" -g '!world_persona.go'
Length of output: 837
cardinal/option.go (2)
59-63: LGTM! Good type safety improvement.
The change from int to uint for the seconds parameter is a good improvement as it prevents negative expiration times.
65-72: LGTM! Verify default cache size sufficiency.
The new option for configuring hash cache size is well-documented and properly implemented. However, we should verify if the default 1MB cache size is sufficient for common use cases.
Let's check for any existing performance metrics or discussions about cache size:
✅ Verification successful
Default 1MB hash cache size is well-justified
The default 1MB cache size appears sufficient based on the codebase analysis:
- The cache is specifically used for replay protection of message signatures
- The default message expiration is 10 seconds, which limits the time window for cached hashes
- The underlying
freecacheimplementation enforces a minimum size of 512KB, and 1MB provides good headroom above this minimum - The size is configurable via
WithHashCacheSizefor cases where larger cache is needed (e.g., with increased message expiration times)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related performance discussions or metrics
rg -i "hash.*cache.*size|cache.*performance" --type md
Length of output: 56
Script:
#!/bin/bash
# Let's search for hash cache size configurations and defaults across the codebase
rg -i "hash.*cache.*size|defaulthashcache" -A 3 -B 3
# Also check for any replay protection or hash cache related tests
fd -e test -e spec -e _test "hash|cache|replay" --exec cat {}
Length of output: 6079
cardinal/server/server.go (7)
17-17: Importing the validator package
The validator package is correctly imported to utilize the SignatureValidator functionality.
27-27: Definition of defaultHashCacheSizeKB constant
The defaultHashCacheSizeKB constant is set to 1024, providing a default hash cache size of 1 MB as indicated by the comment.
31-35: Consider renaming isSignatureValidationDisabled for consistency
As previously suggested, consider renaming isSignatureValidationDisabled to disableSignatureValidation to align with Go's naming conventions for boolean fields.
39-41: Adding validator field to Server struct
The validator field is appropriately added to the Server struct to encapsulate the signature validation logic.
59-63: Initialization of config struct with default values
The default values are correctly assigned to the config struct, ensuring that the server has appropriate initial settings.
70-77: Initialization of SignatureValidator
The SignatureValidator is properly initialized with the configuration parameters and world context. This centralizes the validation logic and improves code maintainability.
187-187: Updating PostTransaction handler with SignatureValidator
The PostTransaction handler is updated to accept the SignatureValidator, aligning with the refactored validation logic and enhancing separation of concerns.
cardinal/server/validator/validator.go (1)
192-192: Simplify Error Wrapping in validateSignature
Wrapping the error returned by tx.Verify(signerAddr) with an empty string using eris.Wrap does not add meaningful context. Consider simplifying the error handling.
cardinal/server/validator/validator_test.go (3)
60-62: Well-structured test suite using testify/suite
Great job setting up the test suite with testify/suite. This approach enhances the organization and readability of your tests.
97-197: Comprehensive testing with signature verification disabled
The test cases thoroughly cover various scenarios when signature verification is disabled, ensuring that transactions are appropriately handled in these conditions.
199-394: Extensive validation tests with signature verification enabled
Your test suite effectively covers a wide range of scenarios, including handling invalid timestamps, altered transaction data, and duplicate transactions. This comprehensive testing enhances the reliability and security of the validation logic.
cardinal/server/handler/tx.go (6)
10-10: Import validator package for signature validation
The addition of the validator package import is appropriate to utilize the SignatureValidator for transaction validation.
36-36: Update function signature to include SignatureValidator
The PostTransaction function now correctly accepts a pointer to SignatureValidator, aligning with the refactored validation logic.
52-54: Consistent error handling with fiber.Error
The use of validator.ValidateTransactionTTL is correct, and logging the internal details is appropriate. Returning a fiber.Error ensures the correct HTTP status code is sent in the response.
107-109: Function PostGameTransaction passes validator correctly
The PostGameTransaction function now correctly delegates to PostTransaction with the validator, ensuring that game transactions are properly validated.
128-130: Function PostPersonaTransaction passes validator correctly
The PostPersonaTransaction function now correctly delegates to PostTransaction with the validator, ensuring that persona transactions are properly validated.
46-46: 🧹 Nitpick (assertive)
Reuse the err variable instead of declaring fiberErr
It's idiomatic in Go to reuse the err variable rather than introducing a new variable like fiberErr. This makes the code cleaner and more consistent.
Apply this diff to reuse the err variable:
- tx, fiberErr := extractTx(ctx, validator)
- if fiberErr != nil {
- return fiberErr
+ tx, err := extractTx(ctx, validator)
+ if err != nil {
+ return errLikely invalid or redundant comment.
Closes: WORLD-1237
- Still need to get rid of the custom ValidationError in favor of eris.wrap
Preview InstructionsTo preview these changes locally, checkout this branch and run: |
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
docs/images/jaeger-ui.pngis excluded by!**/*.png,!**/*.pngdocs/images/prometheus-ui.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (5)
.github/workflows/release-nakama.yaml(1 hunks)docs/cardinal/game/configuration/evm.mdx(1 hunks)docs/cardinal/game/configuration/nakama.mdx(2 hunks)docs/client/nakama/monitoring-nakama.mdx(1 hunks)docs/mint.json(1 hunks)
🔇 Additional comments (5)
docs/cardinal/game/configuration/nakama.mdx (2)
8-8: LGTM! Improved type safety for ENABLE_ALLOWLIST.
Converting from string "false" to boolean false is a good improvement for type safety and configuration clarity.
8-8: Verify backward compatibility of the boolean configuration.
Since this changes the type of ENABLE_ALLOWLIST from string to boolean, ensure that:
- Existing deployment configurations are updated accordingly
- Any configuration parsing code handles both formats during transition
Also applies to: 22-22
✅ Verification successful
Let me gather more information to understand the context better.
Let me check the actual parsing logic to understand how the configuration is handled.
Based on the previous search results, I can now provide a final response:
Boolean configuration change is safely handled with backward compatibility
The codebase already handles the boolean configuration correctly:
- In
e2e/testgames/world.toml, the value is quoted as a string:ENABLE_ALLOWLIST="false" - In
docker-compose.ymlanddocker-compose.benchmark.yml, it uses environment variable substitution with default value:${ENABLE_ALLOWLIST:-false} - The Go code in
relay/nakama/rpc.goexplicitly parses the string value usingstrconv.ParseBool, showing proper type conversion handling - The documentation update in
nakama.mdxreflects the actual runtime behavior rather than introducing a breaking change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining string-based ENABLE_ALLOWLIST usage in configuration files
rg -g '*.{yaml,yml,toml,env,conf}' '"false"|"true".*ENABLE_ALLOWLIST|ENABLE_ALLOWLIST.*"false"|"true"'
# Search for ENABLE_ALLOWLIST parsing logic to verify type handling
ast-grep --pattern 'ENABLE_ALLOWLIST'
Length of output: 430
Script:
#!/bin/bash
# Search for all occurrences of ENABLE_ALLOWLIST to understand its usage
rg "ENABLE_ALLOWLIST" -A 3 -B 3
# Look for any configuration parsing code
ast-grep --pattern 'config.$$$ENABLE_ALLOWLIST$$$'
# Search for any environment variable loading code
rg "process\.env\.ENABLE_ALLOWLIST|os\.getenv.*ENABLE_ALLOWLIST" -A 3 -B 3
Length of output: 4059
Script:
#!/bin/bash
# Look for the configuration parsing in the nakama allowlist package
rg -t go "strconv\.ParseBool.*ENABLE_ALLOWLIST" -A 5 -B 5
# Check for any other boolean parsing related to this configuration
rg -t go "ENABLE_ALLOWLIST.*bool|bool.*ENABLE_ALLOWLIST" -A 5 -B 5
# Look for the initialization of this configuration
ast-grep --pattern 'var $_ = os.Getenv("ENABLE_ALLOWLIST")'
Length of output: 190
docs/client/nakama/monitoring-nakama.mdx (1)
14-20: 🧹 Nitpick (assertive)
Add information about individual container control.
The documentation should clarify if users can start Jaeger or Prometheus individually, and if so, provide the relevant commands.
✅ Verification successful
Based on the codebase analysis, I can now generate the final response:
Individual container control is not supported, but containers can be configured separately
While the --telemetry flag starts both Jaeger and Prometheus containers together, you can control their behavior individually through configuration:
- Jaeger tracing can be enabled/disabled via
NAKAMA_TRACE_ENABLED(defaults to true) - Prometheus metrics can be enabled/disabled via
NAKAMA_METRICS_ENABLED(defaults to true)
Example configuration in world.toml:
NAKAMA_TRACE_ENABLED = false # Disables only Jaeger
NAKAMA_METRICS_ENABLED = false # Disables only Prometheus🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the CLI supports individual container control
world cardinal start --help | grep -A 5 telemetry
Length of output: 124
Script:
#!/bin/bash
# Search for CLI command documentation or implementation related to telemetry
rg -i "telemetry|jaeger|prometheus" -A 5 -B 5
# Also search for any configuration or documentation files
fd -e md -e mdx -e yaml -e toml -e json | xargs rg -i "telemetry|jaeger|prometheus" -A 5 -B 5
Length of output: 90009
.github/workflows/release-nakama.yaml (1)
58-59: LGTM! Verify Dockerfile paths with new build context.
The change to use root context (.) with explicit Dockerfile path is valid and aligns with the codebase restructuring. However, ensure that all COPY/ADD commands in ./relay/nakama/Dockerfile are updated to use correct paths relative to the repository root.
Let's verify the Dockerfile paths:
✅ Verification successful
COPY paths are already correctly configured for root context
The Dockerfile's COPY commands are already using paths relative to the repository root:
COPY relay/nakama .COPY relay/nakama/local.yml /nakama/data/
This confirms that the change to use root context (.) is safe and won't break the build.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check COPY/ADD commands in Dockerfile to ensure paths are correct relative to root context
# Test: Extract and display COPY/ADD commands from Dockerfile
rg -A 1 "^(COPY|ADD)\s" "./relay/nakama/Dockerfile"
Length of output: 200
docs/mint.json (1)
157-158: LGTM! Note about scope.
The addition of the "monitoring-nakama" page to the navigation structure is well-placed and maintains the JSON format. However, this documentation update appears to be outside the scope of the PR's primary objective of refactoring the validator code.
Let's verify the referenced documentation file exists:
✅ Verification successful
Documentation file exists and aligns with navigation update
The verification confirms that the referenced documentation file monitoring-nakama.mdx exists at the expected location docs/client/nakama/, validating the navigation update in mint.json. The change is properly implemented and maintains documentation integrity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the new documentation file
fd "monitoring-nakama.mdx" docs/client/nakama/
Length of output: 87
- got rid of custom error class, using error instead - no longer passing back an http status code from validator - Using Eris to wrap errors with more details - http result code is now determined in handler code based on type of error returned by validator
Preview InstructionsTo preview these changes locally, checkout this branch and run: |
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
cardinal/server/handler/tx.go(7 hunks)cardinal/server/validator/validator.go(1 hunks)cardinal/server/validator/validator_test.go(1 hunks)
🔇 Additional comments (6)
cardinal/server/validator/validator.go (5)
15-21: Add Documentation Comment for Exported Interface SignerAddressProvider
The exported interface SignerAddressProvider lacks a documentation comment. Providing a comment enhances clarity and adheres to Go's documentation standards.
29-30: Typographical Error in Comment
There's a typo in the comment. The word "enought" should be "enough".
85-85: Typographical Error in Comment
There's a typo in the comment. The word "validator" should be "validate".
74-107: Robust Implementation of ValidateTransactionTTL Function
The ValidateTransactionTTL function effectively checks for message expiration, future timestamps, and duplicate messages using the hash cache. The logic is sound, and error handling covers all necessary cases.
109-153: Comprehensive Signature Validation in ValidateTransactionSignature
The ValidateTransactionSignature function correctly handles signature verification, including retrieving the signer address when not provided, and adds the transaction hash to the cache upon successful validation. The conditional checks for when validation is disabled are appropriately handled.
cardinal/server/validator/validator_test.go (1)
294-294: 🧹 Nitpick (assertive)
Fix typographical error in comment
Correct the typo in the comment for clarity.
Apply this diff:
-// TestRejectsAlteredHashTx tests that a transaction with a hashes that is altered after signing is rejected
+// TestRejectsAlteredHashTx tests that a transaction with a hash that is altered after signing is rejectedLikely invalid or redundant comment.
Preview InstructionsTo preview these changes locally, checkout this branch and run: |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
cardinal/server/handler/tx.go(8 hunks)cardinal/server/validator/validator.go(1 hunks)
🔇 Additional comments (4)
cardinal/server/validator/validator.go (4)
74-107: ValidateTransactionTTL function is well-implemented
The ValidateTransactionTTL function correctly checks for message expiration, future timestamps, and duplicate messages using the hash cache. The logic is clear and follows best practices.
109-153: ValidateTransactionSignature function effectively validates transaction signatures
The ValidateTransactionSignature function appropriately handles signature validation, including checking for the presence of a persona tag, retrieving the signer address if necessary, and adding the transaction hash to the cache upon successful validation. The error handling is comprehensive.
155-167: isHashInCache function correctly checks for duplicate messages
The isHashInCache function efficiently determines if a transaction hash exists in the cache, properly handling different error cases from the cache retrieval.
169-175: validateSignature function properly validates the transaction signature
The validateSignature function ensures that the transaction's namespace matches the expected namespace and verifies the signature using the provided signer address.
Closes: WORLD-1237
Overview
Separates out the validation code into SignatureValidator object in a server/validator subpackage to reduce complexity and improve separation of concerns, and makes it independently testable.
Brief Changelog
Testing and Verifying
All CI tests have been run and passed, including the new cardinal/server/validator/validator_test.go suite
Summary by CodeRabbit
Release Notes
New Features
SignatureValidatorfor improved transaction validation, enhancing security against replay attacks.Bug Fixes
Documentation
DA_AUTH_TOKENconfiguration parameter.ENABLE_ALLOWLISTsetting from a string to a boolean in the Nakama configuration documentation.Tests