Permissioned Domains (XLS-80d) #5161
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5161 +/- ##
========================================
Coverage 77.9% 77.9%
========================================
Files 782 786 +4
Lines 66622 66800 +178
Branches 8136 8138 +2
========================================
+ Hits 51902 52067 +165
- Misses 14720 14733 +13
... and 4 files with indirect coverage changes |
oleks-rip
force-pushed
the
pd_xls80
branch
2 times, most recently
from
4029ac5
to
4aede12
Compare
There was a problem hiding this comment.
You need to fix mac errors; these are happening because clang 15 does not fully implement construction of aggregates in the context where std::construct_at needs them to work, which is ret.emplace_back in src/test/jtx/impl/permissioned_domains.cpp, trying to create AuthorizeCredentials defined in src/test/jtx/deposit.h
Here's small repro with this compiler version, to help you experiment with workarounds https://godbolt.org/z/MKfYfsvan
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
oleks-rip
force-pushed
the
pd_xls80
branch
2 times, most recently
from
188fd2b
to
e8ca019
Compare
|
oleks-rip
force-pushed
the
pd_xls80
branch
3 times, most recently
from
391e266
to
99fef88
Compare
| @@ -503,7 +503,8 @@ Env::autofill(JTx& jt) | |||
| } | |||
| catch (parse_error const&) | |||
| { | |||
| test.log << "parse failed:\n" << pretty(jv) << std::endl; | |||
| if (!parseFailureExpected_) | |||
There was a problem hiding this comment.
What is the objective of this change? Just making it easier to identify errors?
There was a problem hiding this comment.
It add too much of false-positive unnecessary output to UT logs which complicates looking for real problem.
There was a problem hiding this comment.
Should there be additional BEAST_EXPECT logic to ensure that an error is actually thrown here?
There was a problem hiding this comment.
It is added in the test, This one is ENV fixing.
| // helpers | ||
| // Make json for PermissionedDomainSet transaction | ||
| Json::Value | ||
| setTx( |
There was a problem hiding this comment.
| setTx( | |
| set( |
There was a problem hiding this comment.
Then you need to rename deleteTx to delete which will conflict with keyword
| if (isUnique_) | ||
| { | ||
| unsigned i = 0; | ||
| for (auto const& cred : sorted) |
There was a problem hiding this comment.
Why not just use a normal for loop with i here? Easier to ensure that all the indices are kept straight.
There was a problem hiding this comment.
using .size() of general container in the loop is bad idea as many of them non const. Adding size variable or something will lead to the similar code.
| if (tx.getTxnType() != ttPERMISSIONED_DOMAIN_SET || result != tesSUCCESS) | ||
| return true; |
There was a problem hiding this comment.
Is this needed? Potentially is harder to remember to update if e.g. there's another transaction later that updates PDs.
There was a problem hiding this comment.
I believe Mark put it here to be consistent with other finalize functions (they have such a check)
Also, test sorting of permissions.
failing the transaction. Conforms to Credentials spec.
High Level Overview of Change
Implements the object, transactions, and tests required by the spec: XLS-80d
Context of Change
New feature. Follows existing patterns for adding a new ledger object and related transactions.
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
Requires an amendment.
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)