Merged
Conversation
Before #5224, pseudoaccount ID was calculated using prefix expressed in std::uint16_t. The refactoring to move the pseudoaccount ID calculation to View.cpp had accidentally changed the prefix type to int (derived from `auto i = 0`) which in turn changed the length of the input to `sha512Half` from 2 bytes to 4, altering the result. This resulted in a different ID of the pseudoaccount calculated from the function after the refactoring, breaking the ledger. This impacts AMMCreate, even when SingleAssetVault amendment is not active.
bthomee
approved these changes
May 23, 2025
vlntb
approved these changes
May 23, 2025
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5447 +/- ##
=======================================
Coverage 78.5% 78.5%
=======================================
Files 813 813
Lines 70085 70086 +1
Branches 8282 8279 -3
=======================================
+ Hits 55033 55043 +10
+ Misses 15052 15043 -9
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
This was referenced Jun 12, 2025
Merged
Closed
Closed
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Before #5224, pseudo-account ID was calculated using prefix expressed in std::uint16_t. #5224 included refactoring to move the pseudo-account ID calculation to
View.cpp, so this calculation can be reused inVaultCreate. This refactoring had accidentally changed the prefix type to int (derived fromauto i = 0) which in turn changed the length of the first input tosha512Halffrom 2 bytes to 4, altering the calculation result.This resulted in a different ID of the pseudo-account calculated from the function after the refactoring, breaking the ledger. This impacts
AMMCreate, even whenSingleAssetVaultamendment is not active.Context of Change
In #5224 I accidentally altered the calculation of pseudo-account ID, even when
SingleAssetVaultamendment is not active. This is obviously breaking the ledger, unintentionally making #5224 a "breaking change". This fix brings back the old calculation.Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)