Permission DEX #5404
Permission DEX #5404
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5404 +/- ##
=========================================
+ Coverage 78.1% 78.3% +0.2%
=========================================
Files 795 796 +1
Lines 68598 68838 +240
Branches 8283 8230 -53
=========================================
+ Hits 53580 53886 +306
+ Misses 15018 14952 -66
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Please fix the clang-format and clang builds |
| namespace test { | ||
| namespace jtx { | ||
|
|
||
| class PermissionedDEX |
There was a problem hiding this comment.
Most of the helper functions here just create/update transaction's JSON. This one is highly specialized and don't belong here, better implement it in cpp file with the tests that used it. Hardcoded accounts and transactions will make explicit and implicit conflicts if someone wants to use this object in other tests.
There was a problem hiding this comment.
This class will be used across multiple test files. It's intentional to hardcode these accounts and transactions, otherwise, there will be a lot of redundant initialization that takes a lot space for every unit test. This object is not meant to be used with other kind of tests which will conflict with it.
There was a problem hiding this comment.
Ok, then at least please rename the accounts to not commonly used names in the tests, like add prefix to them or something
gw("permdex-gateway")
, alice("permdex-alice")
, bob("permdex-bob")
...
|
Would you mind adding the spec link to the PR? If the implementation differs from the spec, could you please clarify the differences? |
shawnxie999
added
Feature Request
src/test/app/Path_test.cpp
Outdated
| @@ -226,6 +239,31 @@ class Path_test : public beast::unit_test::suite | |||
| return std::make_tuple(std::move(paths), std::move(sa), std::move(da)); | |||
| } | |||
|
|
|||
| uint256 | |||
| setupDomain(jtx::Env& env, std::vector<jtx::Account> const& accounts) | |||
There was a problem hiding this comment.
there's some duplicate code in permission_dex.cpp, can we add a common function for setupDomain in permission_dex and call that function?
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)