-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: use zod directly for validating dispute templates #1635
base: dev
Are you sure you want to change the base?
Conversation
WalkthroughThese updates predominantly focus on enhancing the validation and schema structure within the Kleros SDK. By introducing functions like Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- kleros-sdk/src/dataMappings/utils/disputeDetailsSchema.ts (2 hunks)
- kleros-sdk/src/dataMappings/utils/populateTemplate.ts (1 hunks)
Additional comments not posted (4)
kleros-sdk/src/dataMappings/utils/populateTemplate.ts (2)
3-3
: Import of DisputeDetailsSchema approved.The change from a custom validation function to using Zod's schema directly is a modern approach to validation, leveraging Zod's capabilities for robust type checking and error handling.
9-12
: Validation logic and error handling are robust.Using
safeParse
for validation and explicitly handling errors with detailed logging is a strong practice, ensuring that any validation issues are caught and reported clearly.kleros-sdk/src/dataMappings/utils/disputeDetailsSchema.ts (2)
5-7
: Validation functionsisHexAddress
andisHexId
are correctly implemented.The regex patterns used are appropriate for their respective checks. This ensures that only valid hex strings are accepted.
39-67
: Making fields optional aligns with flexible schema requirements.The decision to make several fields optional likely reflects a need for flexibility in the data accepted by these schemas. This can accommodate various use cases without compromising data integrity.
arbitrableAddress: z.string(), // should be changed for ethAddressSchema eventually | ||
arbitratorChainID: z.string(), | ||
arbitratorAddress: ethAddressSchema, | ||
arbitratorAddress: z.string(), // should be changed for ethAddressSchema eventually |
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.
Track temporary changes to address fields.
The temporary use of z.string()
for arbitrableAddress
and arbitratorAddress
should be tracked to ensure they are updated to use ethAddressSchema
in the future.
Consider creating a GitHub issue or a TODO comment in the code to track this necessary change.
Code Climate has analyzed commit 98846e5 and detected 0 issues on this pull request. View more on Code Climate. |
|
This is a WIP:
For now, only escrow v2 and curate v2 disputes render, and all the other ones don't since they don't adhere to the validations I put (which are provisional ones). I think we need to decide on a more robust dispute template specification (which fields exactly the templates will have), and use that dispute template everywhere (on the Dispute Resolver, and on the arbitrables' dispute templates). Also document this once its done so the integrations are very easy.
I've put some logs detailing exactly why the template validation failed in case it did.
Summary by CodeRabbit
New Features
isHexAddress
andisHexId
functions.Improvements
attachment
,metadata
,lang
).Refactor
DisputeDetailsSchema
for improved consistency and error handling.PR-Codex overview
This PR updates dispute details validation by replacing
DisputeDetailsValidator
withDisputeDetailsSchema
for better error handling and adds optional fields to various schemas.Detailed summary
DisputeDetailsValidator
withDisputeDetailsSchema
for validationAnswerSchema
andAliasSchema
attachment
,metadata
,arbitrableAddress
,arbitratorAddress
,lang
,specification
optional inDisputeDetailsSchema