Relocate RPCErr to ripple/protocol - fix previous PR

XRPLF / rippled

Decentralized cryptocurrency blockchain daemon implementing the XRP Ledger protocol in C++

https://xrpl.org

ISC License

4.48k stars 1.45k forks source link

Relocate RPCErr to ripple/protocol - fix previous PR #4940

Closed legleux closed 3 months ago

legleux commented 3 months ago

High Level Overview of Change

Adds STCurrency.h to libxrpl for Clio Fixes PR #4885 botched by force push

Context of Change

Force push in updating PR #4885 undid intended changes. Latest Clio build requires STCurrency.h in libxrpl package.

API Impact

[ ] Public API: New feature (new methods and/or new fields)
[ ] Public API: Breaking change (in general, breaking changes should only impact the next api_version)
[x] libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
[ ] Peer protocol change (must be backward compatible or bump the peer protocol version)

codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 61.62%. Comparing base (2ffead7) to head (b8f55c8).

:exclamation: Current head b8f55c8 differs from pull request most recent head d985810. Consider uploading reports for the commit d985810 to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #4940 +/- ## ============================================ - Coverage 76.94% 61.62% -15.32% ============================================ Files 1127 806 -321 Lines 131675 70967 -60708 Branches 39591 36690 -2901 ============================================ - Hits 101313 43732 -57581 + Misses 24448 19882 -4566 - Partials 5914 7353 +1439 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

legleux commented 3 months ago

2. beast/unit_test/main.cpp

@ximinez This is one thing I was unsure about whether it actually belonged or not. What would be the utility of providing this or not whether installed to a system or provided in a Conan package with libxrpl?

I think @thejohnfreeman has the intention to export all these via globs so that none are forgotten about anymore but that would entail the audit Ed refers to. Should I just close this PR or is that audit/export on too hazy of a timeline?

ximinez commented 3 months ago

This is one thing I was unsure about whether it actually belonged or not. What would be the utility of providing this or not whether installed to a system or provided in a Conan package with libxrpl?

I can't think of much utility. IIRC, if one were to build and run beast/unit_test/main.cpp, it would only be able run the unit tests that are part of beast. Those are all already included in rippled AFAIK. It might be useful as sample / example code, but that can just as easily be obtained from Github.

Should I just close this PR or is that audit/export on too hazy of a timeline?

I vote to keep this PR open. It's a smaller subset of changes that can be reviewed and merged quickly, and I believe unblocks something that @thejohnfreeman is working on.

thejohnfreeman commented 3 months ago

@ximinez we're already installing some headers in impl directories. I would like to hold off on moving them around. My plan is to remove all impl directories in the PR that separates libxrpl and xrpld sources.

ximinez commented 3 months ago

@ximinez we're already installing some headers in impl directories. I would like to hold off on moving them around. My plan is to remove all impl directories in the PR that separates libxrpl and xrpld sources.

That's fine with me