Permissioned DEX #5404
Permissioned DEX #5404
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5404 +/- ##
=========================================
+ Coverage 78.7% 78.8% +0.2%
=========================================
Files 816 817 +1
Lines 70542 70782 +240
Branches 8284 8235 -49
=========================================
+ Hits 55493 55790 +297
+ Misses 15049 14992 -57
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Please fix the clang-format and clang builds |
|
Would you mind adding the spec link to the PR? If the implementation differs from the spec, could you please clarify the differences? |
| if (sleOffer->getFieldH256(sfDomainID) != domainID) | ||
| return false; // LCOV_EXCL_LINE | ||
|
|
||
| if (sleOffer->isFlag(lsfHybrid) && |
There was a problem hiding this comment.
We're checking this in InvariantCheck.cpp so maybe this is redundant?
There was a problem hiding this comment.
That's intentional. Invariant is meant to serve as a defensive check
| if (tx_json.isMember(jss::domain)) | ||
| { | ||
| uint256 num; | ||
| if (!tx_json[sfDomainID.jsonName].isString() || |
There was a problem hiding this comment.
jsonName is unnecessary here and below
There was a problem hiding this comment.
jsonName is necessary here, unless the field is using jss::
There was a problem hiding this comment.
try removing it inside the indexing operator[], you will see :) It is indeed needed for isMember().
| @@ -222,6 +223,22 @@ checkPayment( | |||
| rpcINVALID_PARAMS, | |||
| "Cannot specify both 'tx_json.Paths' and 'build_path'"); | |||
|
|
|||
| std::optional<uint256> domain; | |||
| if (tx_json.isMember(jss::domain)) | |||
There was a problem hiding this comment.
we're mxing jss::domain and sfDomainID, it seems to be a bug and it's not unit tested
There was a problem hiding this comment.
This API is deprecated anyways. I'll change it to sfDomainID. Unit test is not really important here.
There was a problem hiding this comment.
I'd suggest using jss::domain instead of sfDomainID to maintain consistency
| if (!jvParams[jss::domain].isString() || | ||
| !num.parseHex(jvParams[jss::domain].asString())) | ||
| { | ||
| jvStatus = rpcError(rpcDOMAIN_MALFORMED); |
There was a problem hiding this comment.
It'd be better if it can be covered by unit tests
There was a problem hiding this comment.
i think we're currently lacking unit tests for this RPC overall, it'd be good for future work. However RPCs in general are less important than transactions, which should have full coverage
|
Commit message |
High Level Overview of Change
https://github.com/XRPLF/XRPL-Standards/pull/281/files#diff-35a11d4d87e9deea6056a8e974075ece34ffb081689d0afb8a85b6d9bcded756
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)