-
Notifications
You must be signed in to change notification settings - Fork 46
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
chore(sdk): making the curate v2 mappings work #1619
Conversation
WalkthroughThe recent updates mainly enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SDK as kleros-sdk
participant JsonAction as jsonAction
participant ResultObj as createResultObject
User->>SDK: Call jsonAction with mapping
SDK->>JsonAction: jsonAction(mapping)
JsonAction->>JsonAction: Parse value if string
JsonAction-->>ResultObj: createResultObject(parsedValue, seek, populate)
ResultObj->>JsonAction: Return result
JsonAction->>SDK: Return result
SDK->>User: Provide processed result
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 failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2 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-university failed. Why did it fail? →
|
✅ 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: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- kleros-sdk/src/dataMappings/actions/jsonAction.ts (1 hunks)
- kleros-sdk/src/dataMappings/executeActions.ts (1 hunks)
- kleros-sdk/src/dataMappings/utils/createResultObject.ts (1 hunks)
- kleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- kleros-sdk/src/dataMappings/executeActions.ts
Additional context used
GitHub Check: CodeQL
kleros-sdk/src/dataMappings/utils/createResultObject.ts
[failure] 8-8: Incomplete string escaping or encoding
This replaces only the first occurrence of "]".
Biome
kleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.ts
[error] 5-5: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 10-12: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 13-15: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
Additional comments not posted (1)
kleros-sdk/src/dataMappings/utils/createResultObject.ts (1)
2-21
: The changes to accept arrays forseek
andpopulate
enhance the function's flexibility in handling data mappings. The addition ofgetNestedValue
is a solid improvement for accessing nested properties.Tools
GitHub Check: CodeQL
[failure] 8-8: Incomplete string escaping or encoding
This replaces only the first occurrence of "]".
kleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.ts
Outdated
Show resolved
Hide resolved
kleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.ts
Outdated
Show resolved
Hide resolved
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
Outside diff range and nitpick comments (1)
kleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.ts (1)
Line range hint
5-5
: ReplacehasOwnProperty
withObject.hasOwn
for better compliance and safety.The use of
hasOwnProperty
directly on objects is generally discouraged due to potential issues with prototype chain interference. UsingObject.hasOwn
is safer and more modern.- if (context.hasOwnProperty(variableName)) { + if (Object.hasOwn(context, variableName)) {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- kleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.ts (1 hunks)
Additional context used
Biome
kleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.ts
[error] 5-5: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 7-9: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
Additional comments not posted (1)
kleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.ts (1)
12-13
: Parsing logic for nested JSON strings is correctly implemented.The use of regex to replace nested JSON strings with actual JSON objects before parsing is a clever way to handle cases where JSON strings are embedded within JSON strings.
…ses in case the input is a stri
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 (3)
- kleros-sdk/src/dataMappings/actions/jsonAction.ts (1 hunks)
- kleros-sdk/src/dataMappings/utils/createResultObject.ts (1 hunks)
- kleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- kleros-sdk/src/dataMappings/actions/jsonAction.ts
Additional context used
Biome
kleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.ts
[error] 7-13: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 9-13: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 11-13: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
GitHub Check: CodeQL
kleros-sdk/src/dataMappings/utils/createResultObject.ts
[failure] 7-7: Incomplete string escaping or encoding
This replaces only the first occurrence of "]".
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.
lgtm
Code Climate has analyzed commit 78dbbf6 and detected 2 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Quality Gate passedIssues Measures |
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- kleros-sdk/src/dataMappings/utils/createResultObject.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- kleros-sdk/src/dataMappings/utils/createResultObject.ts
ready for review.
LATEST UPDATE:
I updated replacePlaceholdersWithValues() to use mustache instead of the direct string manipulation it was doing. I updated the Escrow V2 and Curate V2 mappings to add triple curly braches in some places too, you can find these updated mappings in the links below. contract would need to be updated with the updated templates/mappings too.
This PR makes the curate v2 mappings work. Also allows accessing indexes of an array returned by the subgraph.
How to check that the mappings work:
Curate V2 template and mappings: kleros/curate-v2#33 (comment)
Escrow V2 template and mappings: kleros/escrow-v2#12 (comment)
The idea is that, with just an
externalDisputeID
, we are able to populate all the needed information from any arbitrable, such as escrow v2, curate v2, or any arbitrable that we integrate with. that would be ideal and would make integrations super easyWe should make this SDK super bullet proof and understandable, make it a good dev experience, not only for the projects we integrate with, but also for us while building this.
(OUTDATED...
)
(OUTDATED.... on the SDK, I reverted some of the changes to jsonAction() and replacePlaceholdersWithValues(). basically Aleix reviewed and I realized that my changes are a hacky way to make the curate v2 "json" mapping work, but it will break normal jsons.
context: we upload the item data json and registry metadata json with a different format to the contracts (picture 2) (subgraph link here with the query) and our jsonAction() mapping can't parse that. possible solutions?:
upload the item data json and registry metadata jsons, with the supported format directly, or,
make the jsonAction() mapping more sophisticated and handle the both cases (normal jsons, and stringified jsons with backlashes "") correctly)
IGNORE:
some JSON Mappings I need to paste here to not lose this info, this is for testing purposes, ignore:
template:
data mappings:
PR-Codex overview
The focus of this PR is to improve data mapping functionality in the kleros-sdk by enhancing JSON parsing and placeholder replacement.
Detailed summary
jsonAction.ts
mustache
inreplacePlaceholdersWithValues.ts
createResultObject.ts
Summary by CodeRabbit
New Features
mustache
library, providing more robust and flexible string handling.Bug Fixes
These changes improve the overall functionality and reliability of the data mapping processes.