Skip to content

fix: name the missing CounterpartySignature when a LoanSet is submitted without it - #3532

Open
kuan121 wants to merge 1 commit into
mainfrom
fix/dge-8439-loanset-missing-counterparty-signature
Open

kuan121 wants to merge 1 commit into
mainfrom
fix/dge-8439-loanset-missing-counterparty-signature

Conversation

@kuan121

@kuan121 kuan121 commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator

High Level Overview of Change

submit and submitAndWait now throw a clear ValidationError when a LoanSet is submitted without the counterparty's signature, instead of passing on rippled's unrelated temBAD_SIGNER message.

Jira: DGE-8439

Context of Change

A LoanSet is signed twice: first by the Account, then by the counterparty through signLoanSetByCounterparty. A developer who forgets the second step and submits gets:

Transaction failed, temBAD_SIGNER: Malformed: No signer may duplicate account or other signers.

rippled returns temBAD_SIGNER from LoanSet::preflight when CounterpartySignature is missing outside a Batch (LoanSet.cpp:94-97), and the generic text for that code points at duplicate signers that don't exist. Three teams at the Lending Protocol hackathon lost time to this.

submitRequest, which both submit and submitAndWait call, now throws before sending:

LoanSet requires the counterparty's signature (CounterpartySignature). Use signLoanSetByCounterparty before submitting.

The condition mirrors rippled's: a top-level LoanSet with no CounterpartySignature and without tfInnerBatchTxn. Inner transactions of a Batch are not checked, since rippled lets an inner LoanSet omit the field. validate() and Wallet.sign() are unchanged: the Account signs before the counterparty does, so a LoanSet without CounterpartySignature is valid until it is submitted.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

One behaviour change: for this input, submit used to resolve with engine_result: temBAD_SIGNER, and it now rejects without sending anything. rippled rejects such a LoanSet in every case, so no working code path changes. ValidationError extends XrplError, so submitAndWait callers catching XrplError still catch it.

The existing multi-signing step in lendingProtocol.test.ts that asserted temBAD_SIGNER for an unsigned-by-counterparty LoanSet now asserts the new error.

Did you update HISTORY.md?

  • Yes

Test Plan

Unit tests in test/client/submit.test.ts:

  • A LoanSet signed only by the Account throws the new error, whether submitted as a blob or signed during submit with a wallet.
  • A LoanSet signed by the counterparty is submitted.
  • A Batch whose inner LoanSet has no CounterpartySignature is submitted.

Integration: the updated step in test/integration/transactions/lendingProtocol.test.ts.

The unit test fails on main and passes with the fix. I ran it locally against rippleci/xrpld:develop --standalone (xrpld 3.4.0-rc1), together with the lending, submit, submitAndWait and Batch integration suites. The xrpl unit suite (134 suites, 1346 tests) and lint also pass.

🤖 Generated with Claude Code

…ed without it

rippled rejects a LoanSet without CounterpartySignature in preflight with
temBAD_SIGNER, which xrpl.js surfaced as "No signer may duplicate account
or other signers". submitRequest now throws a ValidationError that names
CounterpartySignature and signLoanSetByCounterparty instead. A LoanSet
flagged tfInnerBatchTxn is exempt, matching rippled.

DGE-8439

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: XRPLF/xrpl.js/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: dc8f6847-9c4a-438a-bd1c-5cf340edfb96
📥 Commits

Reviewing files that changed from the base of the PR and between cfd0fb4 and 80af91b.

📒 Files selected for processing (4)
  • packages/xrpl/HISTORY.md
  • packages/xrpl/src/sugar/submit.ts
  • packages/xrpl/test/client/submit.test.ts
  • packages/xrpl/test/integration/transactions/lendingProtocol.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.


Walkthrough

Submission validation now raises ValidationError when a LoanSet lacks CounterpartySignature. The check excludes inner Batch transactions. Tests cover unsigned and counterparty-signed submissions, including the Batch exception.

Changes

LoanSet Signature Validation

Layer / File(s) Summary
Add the LoanSet signature check
packages/xrpl/src/sugar/submit.ts
submitRequest checks for a missing CounterpartySignature and raises ValidationError. The helper excludes inner Batch transactions and decodes string transaction inputs.
Verify submission outcomes
packages/xrpl/test/client/submit.test.ts, packages/xrpl/test/integration/transactions/lendingProtocol.test.ts, packages/xrpl/HISTORY.md
Client tests cover rejection without the counterparty signature, acceptance with the signature, and the inner Batch exception. The integration test now expects ValidationError instead of temBAD_SIGNER. The changelog records the behavior.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Suggested reviewers: mvadari

Merge Risk: ⚪ Minimal · up to 80af9

The LoanSet submission change is mergeable after normal checks; no unresolved issue is established.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: naming the missing CounterpartySignature for submitted LoanSet transactions.
Description check ✅ Passed The description includes the required overview, context, change type, HISTORY.md status, and test plan. It explains the behavior change, affected APIs, Batch exception, and test coverage.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The change adds a client-side pre-check in submitRequest that throws a clear ValidationError when a top-level LoanSet lacks CounterpartySignature and isn't an inner Batch transaction (checked via tfInnerBatchTxn), replacing the opaque temBAD_SIGNER rippled error. The logic correctly mirrors rippled's LoanSet::preflight condition, and the top-level TransactionType check naturally excludes Batch submissions from the new check (since the submitted blob's type is Batch, not LoanSet), consistent with the stated intent that inner LoanSets aren't checked. HISTORY.md is updated, and unit/integration tests cover the unsigned, counterparty-signed, and Batch-inner-LoanSet cases. I did not find correctness, security, or consistency issues in the added code worth flagging.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant