Skip to content
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

Add fixAMMv1_2 amendment #5176

Merged
merged 8 commits into from
Nov 5, 2024
Merged

Conversation

gregtatcam
Copy link
Collaborator

High Level Overview of Change

Add fixAMMv1_2 amendment:

  • It adds reserve check on AMMWithdraw
  • It generates AMM max offer if changeSpotPriceQuality() fails

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

Test Plan

Added unit test to check reserve requirements on AMMWithdraw.

* Add reserve check on AMM Withdraw
* Try AMM max offer if changeSpotPriceQuality() fails
@gregtatcam gregtatcam force-pushed the amendment/fix-amm-v1-2 branch from 49e68e4 to c60d273 Compare November 1, 2024 13:34
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 77.6%. Comparing base (d57cced) to head (fb32608).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/app/paths/detail/AMMLiquidity.cpp 80.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5176     +/-   ##
=========================================
- Coverage     77.6%   77.6%   -0.0%     
=========================================
  Files          779     779             
  Lines        65975   66006     +31     
  Branches      8170    8172      +2     
=========================================
+ Hits         51187   51211     +24     
- Misses       14788   14795      +7     
Files with missing lines Coverage Δ
include/xrpl/protocol/Feature.h 100.0% <ø> (ø)
src/xrpld/app/tx/detail/AMMClawback.cpp 100.0% <100.0%> (ø)
src/xrpld/app/tx/detail/AMMWithdraw.cpp 96.2% <100.0%> (+0.2%) ⬆️
src/xrpld/app/tx/detail/AMMWithdraw.h 100.0% <ø> (ø)
src/xrpld/app/paths/detail/AMMLiquidity.cpp 84.7% <80.0%> (-0.2%) ⬇️

... and 8 files with indirect coverage changes

Impacted file tree graph

Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kennyzlei kennyzlei added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 1, 2024
@thejohnfreeman
Copy link
Collaborator

Normally these checks are supposed to use the "prior balance", i.e. the balance of the account before the transaction started executing. I just want to note that we're making an exception here. In fact, it would be easier if we could ignore the "prior balance" everywhere. The context for most helper functions is just the ApplyView, which does not track the "prior balance" of any account. (The transaction function in this PR is not a helper, but one day it might be.) Helpers that require a prior balance demand that it be drilled through the caller as a parameter.

@ximinez ximinez removed the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 4, 2024
@ximinez ximinez self-requested a review November 4, 2024 17:53
src/xrpld/app/tx/detail/AMMWithdraw.cpp Outdated Show resolved Hide resolved
src/xrpld/app/tx/detail/AMMWithdraw.cpp Outdated Show resolved Hide resolved
@kennyzlei kennyzlei requested a review from ximinez November 4, 2024 21:42
@kennyzlei kennyzlei added this to the 2.3.0 (Nov 2024) milestone Nov 4, 2024
@gregtatcam gregtatcam added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 5, 2024
@ximinez ximinez merged commit ec61f5e into XRPLF:develop Nov 5, 2024
20 checks passed
This was referenced Nov 6, 2024
vlntb pushed a commit to vlntb/rippled that referenced this pull request Nov 19, 2024
* Add reserve check on AMM Withdraw
* Try AMM max offer if changeSpotPriceQuality() fails
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants