-
Notifications
You must be signed in to change notification settings - Fork 7
[BOOST-5830] add targetContract check in action validation #488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BOOST-5830] add targetContract check in action validation #488
Conversation
🦋 Changeset detectedLatest commit: 899bdfe The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR 💥 An error occurred when fetching the changed packages and changesets in this PR |
WalkthroughUpdates include a submodule pointer change under Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant EventAction
participant Validator as Validation Helpers
Client->>EventAction: validate(actionStep, tx, logs)
rect rgba(220,240,255,0.4)
note over Validator: isActionStepValid
EventAction->>Validator: check tx.to vs actionStep.targetContract
alt targetContract == zeroAddress
note right of Validator: Wildcard — skip address check
Validator-->>EventAction: true
else addresses match
Validator-->>EventAction: true
else addresses do not match
Validator-->>EventAction: false
end
end
alt step valid
rect rgba(240,255,220,0.4)
note over Validator: isActionEventValid / filterLogsByActionStepCriteria
loop for each log
EventAction->>Validator: compare log.address to targetContract
alt targetContract == zeroAddress
note right of Validator: Wildcard — accept any address
Validator-->>EventAction: include
else address matches
Validator-->>EventAction: include
else address does not match
Validator-->>EventAction: skip
end
end
end
EventAction-->>Client: result based on filtered logs
else step invalid
EventAction-->>Client: false / no match
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Comment |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/sdk/src/Actions/EventAction.ts (1)
1407-1412: LGTM! Address validation is consistent with other methods.The targetContract validation logic is correctly implemented and consistent with
isActionEventValid.Optional refactor: The address-matching logic appears in three locations (lines 822-828, 1009-1016, and 1407-1412). Consider extracting this into a helper method to reduce duplication and improve maintainability.
Example helper method:
private shouldValidateAddress( actionStep: ActionStep, address: Address | null | undefined, ): boolean { if (actionStep.targetContract === zeroAddress) { return true; // wildcard - skip validation } if (!address) { return false; } return isAddressEqual(address, actionStep.targetContract); }Then use it as:
if (!this.shouldValidateAddress(actionStep, log.address)) { continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/evm/lib/openzeppelin-contracts-upgradeable(1 hunks)packages/sdk/src/Actions/EventAction.ts(3 hunks)
🔇 Additional comments (2)
packages/sdk/src/Actions/EventAction.ts (2)
822-828: LGTM! Correctly implements targetContract validation with wildcard support.The address-matching logic properly validates that log addresses match the targetContract, with zeroAddress serving as a wildcard to skip the check. The
continuestatement appropriately filters out non-matching logs.
1009-1016: LGTM! Correctly handles targetContract validation for function calls.The implementation properly validates transaction.to against targetContract with zeroAddress wildcard support. The null check for
transaction.tois good defensive programming.Note: Contract creation transactions (where
transaction.tois null) will fail validation unlesstargetContractis set to zeroAddress, which is the expected behavior.
|
Description
Summary by CodeRabbit
Bug Fixes
Chores