-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: Add rounding to Vault invariants (#6217) #6632
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
Changes from all commits
b7fcb63
a1bfcf3
e1e19d5
f5c9da1
31be15d
0c060dc
9fd7903
cf339fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| #include <xrpl/protocol/STTx.h> | ||
| #include <xrpl/protocol/TER.h> | ||
|
|
||
| #include <optional> | ||
| #include <unordered_map> | ||
| #include <vector> | ||
|
|
||
|
|
@@ -60,18 +61,34 @@ class ValidVault | |
| Shares static make(SLE const&); | ||
| }; | ||
|
|
||
| public: | ||
| struct DeltaInfo final | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's worth implementing arithmetic operations for this class so that we can simplify the code a lot.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I don't see what you did here? |
||
| { | ||
| Number delta = numZero; | ||
| std::optional<int> scale; | ||
|
Tapanito marked this conversation as resolved.
|
||
|
|
||
| // Compute the delta between two Numbers, taking the coarsest scale | ||
| [[nodiscard]] static DeltaInfo | ||
| makeDelta(Number const& after, Number const& before, Asset const& asset); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having the parameters ordered "after" then "before" is a little bit unintuitive. For a function, I'd expect more "before, after". I'll concede that changing it now is going to be a little bit of a pain in the neck...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's bad either way.. strong types would be the ideal solution. Right now you can pass two Numbers in any order and it will compile just fine. I also don't think we need to fix that right now though. |
||
| }; | ||
|
|
||
| private: | ||
| std::vector<Vault> afterVault_ = {}; | ||
| std::vector<Shares> afterMPTs_ = {}; | ||
| std::vector<Vault> beforeVault_ = {}; | ||
| std::vector<Shares> beforeMPTs_ = {}; | ||
| std::unordered_map<uint256, Number> deltas_ = {}; | ||
| std::unordered_map<uint256, DeltaInfo> deltas_ = {}; | ||
|
|
||
| public: | ||
| void | ||
| visitEntry(bool, std::shared_ptr<SLE const> const&, std::shared_ptr<SLE const> const&); | ||
|
|
||
| bool | ||
| finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); | ||
|
|
||
| // Compute the coarsest scale required to represent all numbers | ||
| [[nodiscard]] static std::int32_t | ||
|
Tapanito marked this conversation as resolved.
|
||
| computeCoarsestScale(std::vector<DeltaInfo> const& numbers); | ||
| }; | ||
|
|
||
| } // namespace xrpl | ||
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.
You can't just add
static TYPE constexprwhen you seeconstexpr static TYPEjust belowOn a serious note though i see we are all over the place with this in the codebase and this should probably be addressed by clang-tidy/format instead.