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.
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
feat: LPT freeze #1840
base: develop
Are you sure you want to change the base?
feat: LPT freeze #1840
Changes from 16 commits
b5946ee
da048c4
503ff40
ff5ffae
51ed06b
1822272
7fb6f67
6aeffad
b3c5f2d
78d183c
8500717
4ef03ee
24cfd9a
ece6652
88a3568
696dda3
bba3094
5cb4b87
2ee489a
c102d7a
df29d82
25311ff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
ammAccountHolds and accountHolds seem share some common code , can we refactor them a bit?
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.
i don't think we benefit much from refactoring to share common code. these two functions should be independent from each other anyways
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.
My main thought was that pulling out the common code could make things easier to maintain and avoid duplication. If we ever need to update that logic, we'd only have to do it in one place.
![Screenshot 2025-01-31 at 10 37 36](https://private-user-images.githubusercontent.com/120398799/408538722-0849d412-d648-48f6-bac3-a976889fa950.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkzMTgzMTIsIm5iZiI6MTczOTMxODAxMiwicGF0aCI6Ii8xMjAzOTg3OTkvNDA4NTM4NzIyLTA4NDlkNDEyLWQ2NDgtNDhmNi1iYWMzLWE5NzY4ODlmYTk1MC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjExJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxMVQyMzUzMzJaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1jNDk3N2JjMmJmZTc2OWFhM2M3ZTViNDI4MzRhYWU2YjQ3NGUyMjRmNDgwNjMyY2NkNDRlNWQ5ZDE5NWNkNTc2JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.LEeSL7dPCPljR337-pT5r8tbWS7QaLuL-NqD4ub-b-8)
For example, the green part is the shared code, if there is bug in it, we need to change two places.
Are these two functions really independent? I saw
accountHolds
just does one more check comparing withammAcountHolds
. So the former can call latter like this:`
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.
truthfully, the code in ammAccountHolds:
should be replaced with an assertion
ASSERT(!ripple::isXRP(currency))
, which is why I think these two functions are independentThere 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.
If you genuinely feel certain modifications are necessary, please go ahead and adjust the code accordingly. IMO, refactoring common logic doesn’t mean you can’t make case-specific tweaks; it just helps keep the codebase cleaner and easier to maintain.
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.
they may share some common piece of code now, but that may not be the case soon. In fact i believe the implementation
ammAccountHolds
will soon diverge fromaccountHolds
due to the DeepFreeze feature. For the incoming deepfreeze feature,accountHolds
will need to addisDeepFreeze
check (that's not gated by an amendment).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.
Would it make it impossible for
accountHolds
callsammAccountHolds
?Since you prefer to keep it as is, I'll respect your decision.
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 don't need it if
allowBalance
is false.