Sponsored Content
Skip to content

refactor: Assorted small DID fixes - #6552

Merged
bthomee merged 7 commits into
developfrom
mvadari/addsle-static
Mar 17, 2026
Merged

refactor: Assorted small DID fixes#6552
bthomee merged 7 commits into
developfrom
mvadari/addsle-static

Conversation

@mvadari

@mvadari mvadari commented Mar 16, 2026

Copy link
Copy Markdown
Contributor

High Level Overview of Change

This PR:

  • Makes addSLE in DIDSet a static function, instead of a free function.
  • Renames Attestation to Data everywhere (an artifact of a previous name for the field).
  • Actually runs a set of tests that were not included in the run function of DID_test.

Context of Change

AI code review

Description:
The helper function addSLE is a free function defined in the xrpl namespace with external linkage, no declaration in any header, and no static keyword:

TER
addSLE(ApplyContext& ctx, std::shared_ptr<SLE> const& sle, AccountID const& owner)

This function is logically private to DIDSet.cpp. As written:

  1. It pollutes the xrpl namespace and could conflict with similarly named functions in other TUs.
  2. It creates a potential ODR (One Definition Rule) violation risk.
  3. It can be accidentally called from other translation units that include the right headers.

The fix is to declare it static or move it into an anonymous namespace (as is done consistently throughout the rest of the codebase — see DeleteAccount.cpp which uses namespace { ... } for all its helpers).

API Impact

N/A

@mvadari mvadari added the Trivial Simple change with minimal effect, or already tested. Only needs one approval. label Mar 16, 2026
@mvadari
mvadari requested a review from bthomee March 16, 2026 20:40
@mvadari mvadari changed the title refactor: Make addSLE function static in DIDSet refactor: Assorted small DID fixes Mar 16, 2026
@codecov

codecov Bot commented Mar 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.8%. Comparing base (252c676) to head (1519fdf).
⚠️ Report is 34 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #6552   +/-   ##
=======================================
  Coverage     79.8%   79.8%           
=======================================
  Files          878     878           
  Lines        67893   67893           
  Branches      7550    7550           
=======================================
+ Hits         54210   54211    +1     
+ Misses       13683   13682    -1     
Files with missing lines Coverage Δ
include/xrpl/protocol/Protocol.h 100.0% <ø> (ø)
src/libxrpl/tx/transactors/did/DIDDelete.cpp 100.0% <ø> (ø)
src/libxrpl/tx/transactors/did/DIDSet.cpp 100.0% <100.0%> (ø)

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

@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 17, 2026
@bthomee
bthomee enabled auto-merge (squash) March 17, 2026 14:19
@bthomee
bthomee merged commit 78b2d70 into develop Mar 17, 2026
3 checks passed
@bthomee
bthomee deleted the mvadari/addsle-static branch March 17, 2026 14:44
@mvadari mvadari added the AI Triage Bugs and fixes that have been triaged via AI initiatives label Mar 24, 2026
@mvadari mvadari added this to the 3.2.0 milestone May 20, 2026
beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026
This change:
* Makes `addSLE` in `DIDSet` a static function, instead of a free function.
* Renames `Attestation` to `Data` everywhere (an artifact of a previous name for the field).
* Actually runs a set of tests that were not included in the `run` function of `DID_test`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Triage Bugs and fixes that have been triaged via AI initiatives 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.

2 participants