fix: add hex pattern validation to operator address in definitionSchema#222
fix: add hex pattern validation to operator address in definitionSchema#222aly-obol wants to merge 4 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 48 minutes and 49 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR relaxes address validation in two exported JSON schemas. The ChangesAddress Validation Pattern Updates
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/schema.ts (1)
214-221:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftVerify that empty principal split recipient addresses are handled safely downstream and document the business rule.
The pattern now permits empty strings for
principalSplitRecipients[*].address, but the field remains required (line 221). This creates similar risks as the operator address change:
Data integrity: Required field accepting empty strings may confuse consumers and differs from optional fields.
Inconsistency: Other split recipient addresses in this file (e.g.,
rewardSplitRecipients[*].addressline 164) require strict hex patterns.Downstream safety: From context,
createValidatorManagerAndTotalSplitvalidates withovmTotalSplitPayloadSchemabefore computing split recipients. Empty addresses may cause contract deployment failures or incorrect splits configuration if not explicitly handled.Custom validators: Lines 225-226 apply
validateOVMRewardsSplitRecipientsandvalidateOVMTotalSplitRecipients. Confirm these validators handle empty addresses correctly.Add an inline comment explaining when empty principal split recipient addresses are permitted, and ensure downstream split-creation logic validates or handles empty addresses appropriately.
Run the following script to verify validator implementations and downstream usage:
#!/bin/bash # Description: Check custom validators and principal split recipient address usage # Check OVM validators for empty-string handling echo "=== validateOVMTotalSplitRecipients implementation ===" ast-grep --pattern 'function validateOVMTotalSplitRecipients($$$) { $$$ }' # Check principal split recipient usage in contract/API code echo "=== Principal split recipient usage ===" rg -nC5 --type=ts 'principalSplitRecipients' --glob '!src/schema.ts'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/schema.ts` around lines 214 - 221, The principal split recipients' address currently allows empty strings while being required; update the schema or downstream validators to avoid ambiguous empty required fields: either tighten the schema pattern for principalSplitRecipients[*].address to match non-empty 0x-hex like rewardSplitRecipients, or explicitly document via an inline comment next to the address property when empty string is permitted and why; then ensure the custom validators validateOVMTotalSplitRecipients and validateOVMRewardsSplitRecipients (and the ovmTotalSplitPayloadSchema/check in createValidatorManagerAndTotalSplit) explicitly handle or reject empty addresses and add a short inline comment referencing this business rule so callers know whether empty principal addresses are allowed and how they are handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/schema.ts`:
- Around line 214-221: The principal split recipients' address currently allows
empty strings while being required; update the schema or downstream validators
to avoid ambiguous empty required fields: either tighten the schema pattern for
principalSplitRecipients[*].address to match non-empty 0x-hex like
rewardSplitRecipients, or explicitly document via an inline comment next to the
address property when empty string is permitted and why; then ensure the custom
validators validateOVMTotalSplitRecipients and validateOVMRewardsSplitRecipients
(and the ovmTotalSplitPayloadSchema/check in
createValidatorManagerAndTotalSplit) explicitly handle or reject empty addresses
and add a short inline comment referencing this business rule so callers know
whether empty principal addresses are allowed and how they are handled.
|
1 participant
Summary
pattern: '^0x[a-fA-F0-9]{40}$'to operatoraddressindefinitionSchema, matching the validation already applied to validator addresses, splitter recipients, and every other address field in the schema.0xGGGG...pass the SDK's AJV validation (only a length check existed viavalidateUniqueAddresses). The obol-api server-side@IsEthereumAddress()+getAddress()guards catch these before storage, so the practical impact is low.Test plan
npm test)createClusterDefinitionwith a non-hex operator address (e.g.0xGGGG...GGGG) now throws a validation error from the SDK before hitting the APISummary by CodeRabbit