fix: Remove superfluous view update from credentials - #6545
Conversation
oleks-rip
changed the title
oleks-rip
changed the title
oleks-rip
added
the
Trivial
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6545 +/- ##
=========================================
- Coverage 81.5% 81.4% -0.0%
=========================================
Files 988 988
Lines 74411 74410 -1
Branches 7559 7556 -3
=========================================
- Hits 60609 60606 -3
- Misses 13802 13804 +2
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| if (!page) | ||
| return tecDIR_FULL; | ||
| sleCred->setFieldU64(sfSubjectNode, *page); | ||
| view().update(view().peek(keylet::account(subject))); |
There was a problem hiding this comment.
In what way is this superfluous?
There was a problem hiding this comment.
The subject's account keylet is never modified. The SLE is never even checked out.
See the metadata in this transaction for an example: https://xrpscan.com/tx/991F8E93B4CF0B2D0A9479E76A5BEF3B62B755F075F249EC142A9B942B7E9E5E
There was a problem hiding this comment.
The subject's account keylet is never modified. The SLE is never even checked out. See the metadata in this transaction for an example: xrpscan.com/tx/991F8E93B4CF0B2D0A9479E76A5BEF3B62B755F075F249EC142A9B942B7E9E5E
That may be intentional. By marking the account as modified, it shows up in the metadata. It also gets threaded, and is included in the account_tx RPC lookup. Since the object is added to the subject's directory, it makes sense to me to include the subject's account.
There was a problem hiding this comment.
For example, I'll bet TrustSet does the same thing, though maybe through a different mechanism for the other side when creating a new trust line.
There was a problem hiding this comment.
It doesn't show up in the metadata, though
There was a problem hiding this comment.
It will not show in metadata if curNode == origNode, as it seems to be the case here
There was a problem hiding this comment.
I'm looking at the block above and I see that the issuer has the same logic, but increments counter when we add an object to the account. Should this actually be the same here for subject?
There was a problem hiding this comment.
The counter shouldn't be incremented because it's not the subject that's bearing the reserve cost for the credential upon creation (otherwise you could spam someone with credentials and use up all their XRP in reserves).
There was a problem hiding this comment.
I don't see any problem with this change then.
I will leave it for @ximinez to decide since he raised this question
There was a problem hiding this comment.
@ximinez told me that it is ok to remove the update call if it has no effect. I think we can continue here.
oleks-rip
added
the
Ready to merge
|
/ai-review |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
mvadari
added
the
AI Triage
5 participants
High Level Overview of Change
Subject account doesn't change when credentials where created by issuer, therefore its update is not necessary.
Type of Change
.gitignore, formatting, dropping support for older tooling)