Sponsored Content
Skip to content

Refactor: Update PermissionedDomainDelete to use keylet for sle access - #6063

Merged
bthomee merged 5 commits into
XRPLF:developfrom
tequdev:refactor-pdomain
Mar 4, 2026
Merged

Refactor: Update PermissionedDomainDelete to use keylet for sle access#6063
bthomee merged 5 commits into
XRPLF:developfrom
tequdev:refactor-pdomain

Conversation

@tequdev

@tequdev tequdev commented Nov 21, 2025

Copy link
Copy Markdown
Member

High Level Overview of Change

Refactor to explicitly use keylet::permissionedDomain in PermissionedDomainDelete processing.

Context of Change

Refactor to explicitly use keylet::permissionedDomain in PermissionedDomainDelete processing.

Type of Change

  • Refactor (non-breaking change that only restructures code)

@codecov

codecov Bot commented Nov 21, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.8%. Comparing base (fcec31e) to head (6b12a94).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #6063   +/-   ##
=======================================
  Coverage     79.8%   79.8%           
=======================================
  Files          858     858           
  Lines        67764   67764           
  Branches      7555    7548    -7     
=======================================
+ Hits         54074   54087   +13     
+ Misses       13690   13677   -13     
Files with missing lines Coverage Δ
...rs/PermissionedDomain/PermissionedDomainDelete.cpp 100.0% <100.0%> (ø)

... and 7 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.

Comment on lines +57 to +58
auto const ownerSle = view().peek(keylet::account(account_));
auto const owner = (*slePd)[sfOwner];
auto const ownerSle = view().peek(keylet::account(owner));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why the change here? They are already checked that they are the same in preclaim

@tequdev tequdev Nov 21, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

because it is more intuitive.
Future readers of this code will no longer need to check preclaim.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably better to split it into another PR?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@tequdev, could you please remove this 2 lines change as it looks not necessary here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

8c34610

fixed

@bthomee
bthomee requested a review from a1q123456 November 21, 2025 16:04
@mvadari

mvadari commented Feb 6, 2026

Copy link
Copy Markdown
Contributor

@tequdev can you update the PR description?

@mvadari mvadari added the Trivial Simple change with minimal effect, or already tested. Only needs one approval. label Mar 3, 2026
@mvadari

mvadari commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

@tequdev is this PR ready to go?

@tequdev

tequdev commented Mar 4, 2026

Copy link
Copy Markdown
Member Author

@tequdev is this PR ready to go?

yes

@mvadari mvadari 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 Mar 4, 2026
@bthomee
bthomee merged commit 3cd1e3d into XRPLF:develop Mar 4, 2026
3 checks passed
@ximinez ximinez modified the milestones: 3.1.3, 3.1.3 (develop) Apr 15, 2026
beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026
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. Trivial Simple change with minimal effect, or already tested. Only needs one approval.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants