Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(xrpl): custom definitions support #2683
base: main
Are you sure you want to change the base?
feat(xrpl): custom definitions support #2683
Changes from all commits
23411ac
9249824
b529149
a618fb9
b283fc3
4689b02
59c2074
6089035
7021e63
02a8503
3153659
b047f04
eeb9f43
2900ab3
2c60a1e
071beb5
04446ff
2452093
4731f60
5db85fe
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
🛠️ Refactor suggestion
Avoid overwriting user-provided definitions in
submit
By spreading
...opts
and then settingdefinitions: this.definitions
, anydefinitions
provided by the user inopts
will be overwritten bythis.definitions
. To respect user-supplied definitions, consider prioritizingopts.definitions
:This change ensures that if the user provides
definitions
inopts
, it will be used; otherwise,this.definitions
will be applied.📝 Committable suggestion
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.
🛠️ Refactor suggestion
Consistent handling of definitions in
submitAndWait
Similar to the
submit
method, thesubmitAndWait
method overwrites any user-provideddefinitions
inopts
. Apply the same fix to ensure consistency and respect for user input:📝 Committable suggestion
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.
Could you explain what else is remaining as far as validation is concerned? Are you referring to validation of the
SField
s used inside a transaction ? (This is referred to asFIELDS
in the definitions.json file)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.
Yes, that was the idea. You could introduce further validations in the future, like the one you mentioned.
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.
If I happen to modify the behavior of an existing transaction (say
TicketCreate
), the custom-definitions will not help me. Since this transaction-name already exists in the codebase, the old transaction validation is used.The existing
validate_<transaction_name>
methods are functionally different fromvalidateTxAgainstCustomDefinitions
method. The former enforces theTxFormat
whereas the latter concerns itself with serialization issues.Is my understanding correct ?
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.
🛠️ Refactor suggestion
Refactor 'submitRequest' to use an options object
The function
submitRequest
now accepts four parameters, exceeding the ESLintmax-params
limit. Instead of disabling the ESLint rule, consider refactoring the function to accept an options object for optional parameters. This enhances readability and makes it easier to manage additional parameters in the future.Apply this diff to refactor
submitRequest
:And update the function calls accordingly.