Skip to content

Conversation

tequdev
Copy link
Member

@tequdev tequdev commented Aug 20, 2025

High Level Overview of Change

Blocks incoming (as Subject field) CredentialCreate for your account, similar to the existing DisallowIncoming flags.

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)

@tequdev tequdev requested a review from a team as a code owner August 20, 2025 16:32
{
auto const id = ctx.tx[sfAccount];
if (id != subject &&
sleSubject->getFlags() & lsfDisallowIncomingCredential)
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use isFlag instead of getFlags

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

78e2f03

@Bronek Bronek added this to the 3.0.0 milestone Aug 29, 2025
@mvadari
Copy link
Collaborator

mvadari commented Aug 29, 2025

Shouldn't this be a feature amendment?

Copy link

codecov bot commented Aug 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.9%. Comparing base (5d79bfc) to head (61e7bd4).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5704     +/-   ##
=========================================
- Coverage     78.9%   78.9%   -0.0%     
=========================================
  Files          816     816             
  Lines        72075   72086     +11     
  Branches      8437    8414     -23     
=========================================
+ Hits         56865   56873      +8     
- Misses       15210   15213      +3     
Files with missing lines Coverage Δ
src/xrpld/app/tx/detail/Credentials.cpp 96.7% <100.0%> (+0.1%) ⬆️
src/xrpld/app/tx/detail/SetAccount.cpp 98.4% <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.

@tequdev
Copy link
Member Author

tequdev commented Aug 30, 2025

Shouldn't this be a feature amendment?

@mvadari

I have chosen it as Fix Amendment because I thought it was originally appropriate to include it in the Credentials Amendment.

Changing to Feature Amendment is totally fine.

Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

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

LGTM

@mvadari
Copy link
Collaborator

mvadari commented Sep 2, 2025

I thought it was originally appropriate to include it in the Credentials Amendment.

Fair, but it wasn't in the spec, and I would consider this more of an expansion of the spec than a fix.

This amendment adds a new flag. For people implementing tooling, they're generally paying closer attention to the feature amendments and not the fix amendments, and therefore might miss the addition of the new flag if they don't double check this amendment.

@Bronek Bronek added the Needs second review PR requires at least one more code review approval before it can be merged label Sep 5, 2025
@Bronek Bronek requested a review from mvadari September 5, 2025 15:26
@mvadari
Copy link
Collaborator

mvadari commented Sep 5, 2025

@tequdev please change this to a feature amendment and publish a spec for it in the XRPL-Standards discussions

@tequdev tequdev changed the title fixDisallowIncomingCredential DisallowIncomingCredential Sep 21, 2025
@tequdev
Copy link
Member Author

tequdev commented Sep 21, 2025

Created the spec and changed it to a feature amendment.

XRPLF/XRPL-Standards#379

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Amendment Needs second review PR requires at least one more code review approval before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants