Skip to content

Retire fix1571 Amendment#5925

Merged
bthomee merged 1 commit intodevelopfrom
pratik/Retire_fix1571_amendment
Oct 29, 2025
Merged

Retire fix1571 Amendment#5925
bthomee merged 1 commit intodevelopfrom
pratik/Retire_fix1571_amendment

Conversation

@pratikmankawde
Copy link
Contributor

High Level Overview of Change

This change retires the fix1571 amendment.

Context of Change

Original amendment: https://xrpl.org/resources/known-amendments#fix1571
Amendments activated for more than 2 years can be retired.

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

@pratikmankawde pratikmankawde requested review from a team and ximinez and removed request for a team October 23, 2025 12:02
env(escrow::finish("carol", "alice", seqFt), fee(150 * baseFee));
BEAST_EXPECT(
env.balance("bob") ==
XRP(5200)); // 5100 (from last transaction) + 100
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ximinez Can you please confirm if this new unit test which checks presence of cancel time and finish time combination is correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

considering there's a bunch of tests in this file that have almost same check (search for "Metadata to self" testcase for example), I'd say this seems correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for confirming.

@codecov
Copy link

codecov bot commented Oct 23, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.3%. Comparing base (a8e4da0) to head (898bfa0).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5925   +/-   ##
=======================================
  Coverage     78.3%   78.3%           
=======================================
  Files          817     817           
  Lines        68978   68974    -4     
  Branches      8328    8324    -4     
=======================================
- Hits         54010   54009    -1     
+ Misses       14968   14965    -3     
Files with missing lines Coverage Δ
src/xrpld/app/tx/detail/Escrow.cpp 99.2% <100.0%> (-<0.1%) ⬇️

... and 3 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pratikmankawde pratikmankawde requested review from a1q123456 and vvysokikh1 and removed request for ximinez October 24, 2025 15:11
Copy link
Contributor

@vvysokikh1 vvysokikh1 left a comment

Choose a reason for hiding this comment

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

lgtm

env(escrow::finish("carol", "alice", seqFt), fee(150 * baseFee));
BEAST_EXPECT(
env.balance("bob") ==
XRP(5200)); // 5100 (from last transaction) + 100
Copy link
Contributor

Choose a reason for hiding this comment

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

considering there's a bunch of tests in this file that have almost same check (search for "Metadata to self" testcase for example), I'd say this seems correct

Copy link
Contributor

@a1q123456 a1q123456 left a comment

Choose a reason for hiding this comment

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

LGTM

@pratikmankawde pratikmankawde added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Oct 29, 2025
Amendments activated for more than 2 years can be retired. This change retires the fix1571 amendment.
@bthomee bthomee force-pushed the pratik/Retire_fix1571_amendment branch from 0128477 to 898bfa0 Compare October 29, 2025 13:40
@bthomee bthomee added this pull request to the merge queue Oct 29, 2025
github-merge-queue bot pushed a commit that referenced this pull request Oct 29, 2025
Amendments activated for more than 2 years can be retired. This change retires the fix1571 amendment.

Co-authored-by: Bart Thomee <11445373+bthomee@users.noreply.github.com>
@bthomee bthomee removed this pull request from the merge queue due to a manual request Oct 29, 2025
@bthomee bthomee added this pull request to the merge queue Oct 29, 2025
Merged via the queue into develop with commit bd3bc91 Oct 29, 2025
69 of 71 checks passed
@bthomee bthomee deleted the pratik/Retire_fix1571_amendment branch October 29, 2025 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants