XLS-68d Sponsored Fees and Reserves#921
Conversation
update two incorrect unit tests
…nsor-related fields
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds XLS-68 sponsorship support: new SponsorshipSet and SponsorshipTransfer transactions and flags, SponsorSignature inner object, sponsor fields on Transaction, sponsor-aware fee calculation, binary/ledger definitions for sponsorship, granular sponsor permissions, request/ledger filters, public exports, and many unit/integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Sponsee as Sponsee
participant Sponsor as Sponsor
participant Network as Network
participant Ledger as Ledger
Sponsee->>Sponsor: Request sponsorship / provide sponsee account
Sponsor->>Sponsor: Build SponsorshipSet(tx)
Sponsor->>Network: Submit SponsorshipSet
Network->>Ledger: Validate & execute
Ledger->>Ledger: Create Sponsorship ledger entry
Ledger-->>Network: tesSUCCESS
Network-->>Sponsor: Confirm sponsorship active
sequenceDiagram
participant Sponsee as Sponsee
participant Sponsor as Sponsor
participant Network as Network
participant Ledger as Ledger
Sponsor->>Sponsor: Build SponsorshipTransfer(tx)
Sponsor->>Sponsee: Request sponsor co-signature (SponsorSignature)
Sponsee->>Sponsor: Return SponsorSignature (single or multisig chunks)
Sponsor->>Network: Submit SponsorshipTransfer with SponsorSignature
Network->>Ledger: Validate sponsor + sponsee signatures
Ledger->>Ledger: Update sponsorship ownership
Ledger-->>Network: tesSUCCESS
Network-->>Sponsor: Confirm transfer
sequenceDiagram
participant Txn as Transaction
participant FeeCalc as FeeCalculator
participant Network as Network
participant SponsorAcc as SponsorAccount
Txn->>FeeCalc: Request fee calc (includes sponsor?)
alt sponsor present
FeeCalc->>SponsorAcc: Fetch signer count
SponsorAcc-->>FeeCalc: signer_count
alt signer_count > 1
FeeCalc->>FeeCalc: Warn multi-signer sponsor
FeeCalc->>FeeCalc: base_fee *= (1 + signer_count)
else
FeeCalc->>FeeCalc: Use base fee
end
else
FeeCalc->>FeeCalc: Standard fee calc
end
FeeCalc-->>Txn: Return calculated fee
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
xrpl/models/requests/account_objects.py (1)
69-69: Consider adding a docstring for thesponsoredfield.Other fields in this class have docstrings explaining their purpose. Adding documentation would clarify the filtering behavior (e.g., whether
Truereturns only sponsored objects,Falsereturns non-sponsored, andNonereturns all).📝 Suggested docstring
method: RequestMethod = field(default=RequestMethod.ACCOUNT_OBJECTS, init=False) sponsored: Optional[bool] = None + """ + If True, filter to objects that are sponsored. If False, filter to objects + that are not sponsored. If omitted, return all objects regardless of + sponsorship status. + """ type: Optional[AccountObjectType] = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xrpl/models/requests/account_objects.py` at line 69, Add a docstring for the sponsored field in the AccountObjects request model (the sponsored: Optional[bool] = None attribute) matching the style of the other field docs; describe that sponsored=True filters to only sponsored objects, sponsored=False to only non-sponsored objects, and sponsored=None (or omitted) returns both/all objects, and place this explanatory string immediately above or inline with the sponsored attribute to keep it consistent with the class's existing field documentation.tests/integration/transactions/test_account_set.py (1)
79-111: Consider adding assertion for the set transaction result.Line 92 submits the
set_txbut doesn't assert the result. If this fails silently, the subsequent clear and sponsorship tests could pass for the wrong reasons.🛡️ Suggested improvement
- await sign_and_reliable_submission_async(set_tx, sponsee_wallet, client) + set_resp = await sign_and_reliable_submission_async(set_tx, sponsee_wallet, client) + self.assertEqual(set_resp.result["engine_result"], "tesSUCCESS")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/transactions/test_account_set.py` around lines 79 - 111, The test test_clear_disallow_incoming_sponsor submits the initial AccountSet (variable set_tx) but never checks its result; after calling sign_and_reliable_submission_async(set_tx, sponsee_wallet, client) capture the response (e.g., set_resp) and assert set_resp.result["engine_result"] == "tesSUCCESS" before proceeding to clear_tx and sponsor_tx to ensure the flag was actually set; update references to set_tx and sign_and_reliable_submission_async in the test to perform and assert this check.xrpl/asyncio/transaction/main.py (2)
594-606: Consider usingwarnings.warn()instead ofprint().The warning uses
print()which goes to stdout and may be missed in production environments. This is inconsistent with how proper warnings should be emitted. Consider usingwarnings.warn()for better integration with logging infrastructure, similar to how deprecation warnings are typically handled.♻️ Suggested refactor
+import warnings ... - print( - ( - f"Warning: You are using autofill for a " - f"sponsored transaction: " - f"{transaction.to_dict()}. The fee " - "estimation is based on the number of " - "signers in the sponsor's SignerList. It " - "might be possible to optimize the fee by " - "considering the minimum quorum." - "\nIf you prefer optimized transaction fee," - " please fill the fee field manually." - ) - ) + warnings.warn( + f"Autofill fee estimation for sponsored transaction uses the full " + f"SignerList count ({sponsor_signers_count}). You may optimize by " + f"using the minimum quorum and setting the fee field manually.", + stacklevel=2, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xrpl/asyncio/transaction/main.py` around lines 594 - 606, Replace the print(...) call that emits the sponsored-transaction message with warnings.warn(...) so the warning goes through Python's warning system; specifically, construct the same multi-line message (using the existing f-string with transaction.to_dict()) and call warnings.warn(message, category=UserWarning, stacklevel=2). Also add an import warnings at the top of the module if it isn't already present and remove the print(...) invocation in the block where the transaction autofill for sponsored transactions is handled.
587-587: Minor: Replace ambiguous multiplication sign.Static analysis flagged the
×(MULTIPLICATION SIGN) as ambiguous. Consider replacing withxfor consistency with other comments in the codebase.- # Fee = (1 + |tx.Signers| + |SponsorSignature.Signers|) × base + # Fee = (1 + |tx.Signers| + |SponsorSignature.Signers|) x base🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xrpl/asyncio/transaction/main.py` at line 587, Replace the ambiguous multiplication sign in the comment "Fee = (1 + |tx.Signers| + |SponsorSignature.Signers|) × base" with a plain ASCII "x" so it becomes "Fee = (1 + |tx.Signers| + |SponsorSignature.Signers|) x base"; locate that comment in xrpl/asyncio/transaction/main.py (the line containing the Fee formula) and update the character only—no logic or formatting changes.tests/integration/transactions/test_account_delete.py (1)
78-81: Consider using a more efficient approach for ledger advancement.Advancing 260 ledgers with individual requests may be slow. However, this matches the AccountDelete sequence requirement (
current_ledger_index >= account_sequence + 256), so the approach is functionally correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/transactions/test_account_delete.py` around lines 78 - 81, The test currently advances _LEDGERS_TO_ADVANCE ledgers by looping and calling await client.request(LEDGER_ACCEPT_REQUEST) repeatedly which is inefficient; replace the loop with a single batched advance by using an existing helper or extending the request to accept a count (e.g., call a helper like await advance_ledgers(client, _LEDGERS_TO_ADVANCE) or send one client.request with LEDGER_ACCEPT_REQUEST augmented to include the desired number of ledger accepts), ensuring you reference and use the symbols _LEDGERS_TO_ADVANCE, client.request, and LEDGER_ACCEPT_REQUEST when implementing the change.tests/unit/models/transactions/test_sponsor_signature.py (1)
65-84: Consider consolidating duplicate test methods.
test_valid_sponsor_signature_single_sig(lines 65-71) duplicatestest_valid_with_single_signature(lines 18-24), andtest_valid_sponsor_signature_multi_sig(lines 73-84) duplicatestest_valid_with_signers(lines 26-42). They test the same validation paths with slightly different test data.Consider removing the duplicate methods or consolidating them to reduce maintenance overhead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/transactions/test_sponsor_signature.py` around lines 65 - 84, The two tests test_valid_sponsor_signature_single_sig and test_valid_sponsor_signature_multi_sig duplicate existing tests test_valid_with_single_signature and test_valid_with_signers; remove the redundant methods or consolidate them (e.g., keep only test_valid_with_single_signature and test_valid_with_signers) so each validation path for SponsorSignature (using signing_pub_key/txn_signature and using signers list via Signer with account=_ACCOUNT2) is covered once; update any references to the removed test names and ensure SponsorSignature and Signer usage remains identical in the retained tests.xrpl/models/transactions/sponsorship_set.py (1)
117-133: Multiple flag validation errors overwrite each other.When multiple flag validation errors occur simultaneously, only the last one is preserved because they all use the same
errors["flags"]key. For example, if a user sets bothTF_SPONSORSHIP_SET_REQUIRE_SIGN_FOR_FEEandTF_SPONSORSHIP_CLEAR_REQUIRE_SIGN_FOR_FEE, AND also sets both reserve flags, only the reserve error will be shown.This is a minor UX issue since validation still fails, but the user may not see all problems at once.
🔧 Option to accumulate multiple flag errors
+ flag_errors = [] if set_fee and clear_fee: - errors["flags"] = ( + flag_errors.append( "`TF_SPONSORSHIP_SET_REQUIRE_SIGN_FOR_FEE` and " "`TF_SPONSORSHIP_CLEAR_REQUIRE_SIGN_FOR_FEE` are mutually exclusive." ) if set_res and clear_res: - errors["flags"] = ( + flag_errors.append( "`TF_SPONSORSHIP_SET_REQUIRE_SIGN_FOR_RESERVE`" " and " "`TF_SPONSORSHIP_CLEAR_REQUIRE_SIGN_FOR_RESERVE`" " are mutually exclusive." ) if delete_obj and (set_fee or clear_fee or set_res or clear_res): - errors["flags"] = ( + flag_errors.append( "`TF_DELETE_OBJECT` cannot be combined with any set/clear flags." ) + if flag_errors: + errors["flags"] = " ".join(flag_errors)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xrpl/models/transactions/sponsorship_set.py` around lines 117 - 133, The validation currently overwrites errors["flags"] when multiple mutually-exclusive flag checks fail (see checks using set_fee, clear_fee, set_res, clear_res, delete_obj), so change errors["flags"] to accumulate all messages instead of replacing them: initialize errors["flags"] as a list (or append to it if already present) and push each descriptive message for the three checks, or concatenate messages with separators before finalizing; ensure any downstream consumers that expect a string still receive a joined string if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/models/transactions/test_sponsorship_transfer.py`:
- Around line 68-75: The test docstring for test_valid_reassign_flag is
incorrect: TF_SPONSORSHIP_REASSIGN does not forbid sponsee according to
SponsorshipTransfer._get_errors (only TF_SPONSORSHIP_CREATE forbids sponsee).
Update the docstring in test_valid_reassign_flag to accurately state that
TF_SPONSORSHIP_REASSIGN is a reassignment flag and does not impose the "no
sponsee" restriction (or otherwise describe expected behavior), referencing the
SponsorshipTransfer class and the TF_SPONSORSHIP_REASSIGN/TF_SPONSORSHIP_CREATE
flags.
In `@xrpl/asyncio/transaction/main.py`:
- Around line 586-607: The sponsor fee is only added when sponsor_signers_count
> 1, but per the comment/XLS‑68 it must be applied whenever transaction.sponsor
is not None; call _fetch_counterparty_signers_count(client, transaction.sponsor)
to get sponsor_signers_count and always add base_fee += net_fee * (1 +
sponsor_signers_count) when transaction.sponsor is present, while keeping the
multi‑sign warning (the print) only inside the sponsor_signers_count > 1 branch;
update the block around transaction.sponsor, sponsor_signers_count, base_fee and
net_fee so the fee math is executed unconditionally for any sponsor.
---
Nitpick comments:
In `@tests/integration/transactions/test_account_delete.py`:
- Around line 78-81: The test currently advances _LEDGERS_TO_ADVANCE ledgers by
looping and calling await client.request(LEDGER_ACCEPT_REQUEST) repeatedly which
is inefficient; replace the loop with a single batched advance by using an
existing helper or extending the request to accept a count (e.g., call a helper
like await advance_ledgers(client, _LEDGERS_TO_ADVANCE) or send one
client.request with LEDGER_ACCEPT_REQUEST augmented to include the desired
number of ledger accepts), ensuring you reference and use the symbols
_LEDGERS_TO_ADVANCE, client.request, and LEDGER_ACCEPT_REQUEST when implementing
the change.
In `@tests/integration/transactions/test_account_set.py`:
- Around line 79-111: The test test_clear_disallow_incoming_sponsor submits the
initial AccountSet (variable set_tx) but never checks its result; after calling
sign_and_reliable_submission_async(set_tx, sponsee_wallet, client) capture the
response (e.g., set_resp) and assert set_resp.result["engine_result"] ==
"tesSUCCESS" before proceeding to clear_tx and sponsor_tx to ensure the flag was
actually set; update references to set_tx and sign_and_reliable_submission_async
in the test to perform and assert this check.
In `@tests/unit/models/transactions/test_sponsor_signature.py`:
- Around line 65-84: The two tests test_valid_sponsor_signature_single_sig and
test_valid_sponsor_signature_multi_sig duplicate existing tests
test_valid_with_single_signature and test_valid_with_signers; remove the
redundant methods or consolidate them (e.g., keep only
test_valid_with_single_signature and test_valid_with_signers) so each validation
path for SponsorSignature (using signing_pub_key/txn_signature and using signers
list via Signer with account=_ACCOUNT2) is covered once; update any references
to the removed test names and ensure SponsorSignature and Signer usage remains
identical in the retained tests.
In `@xrpl/asyncio/transaction/main.py`:
- Around line 594-606: Replace the print(...) call that emits the
sponsored-transaction message with warnings.warn(...) so the warning goes
through Python's warning system; specifically, construct the same multi-line
message (using the existing f-string with transaction.to_dict()) and call
warnings.warn(message, category=UserWarning, stacklevel=2). Also add an import
warnings at the top of the module if it isn't already present and remove the
print(...) invocation in the block where the transaction autofill for sponsored
transactions is handled.
- Line 587: Replace the ambiguous multiplication sign in the comment "Fee = (1 +
|tx.Signers| + |SponsorSignature.Signers|) × base" with a plain ASCII "x" so it
becomes "Fee = (1 + |tx.Signers| + |SponsorSignature.Signers|) x base"; locate
that comment in xrpl/asyncio/transaction/main.py (the line containing the Fee
formula) and update the character only—no logic or formatting changes.
In `@xrpl/models/requests/account_objects.py`:
- Line 69: Add a docstring for the sponsored field in the AccountObjects request
model (the sponsored: Optional[bool] = None attribute) matching the style of the
other field docs; describe that sponsored=True filters to only sponsored
objects, sponsored=False to only non-sponsored objects, and sponsored=None (or
omitted) returns both/all objects, and place this explanatory string immediately
above or inline with the sponsored attribute to keep it consistent with the
class's existing field documentation.
In `@xrpl/models/transactions/sponsorship_set.py`:
- Around line 117-133: The validation currently overwrites errors["flags"] when
multiple mutually-exclusive flag checks fail (see checks using set_fee,
clear_fee, set_res, clear_res, delete_obj), so change errors["flags"] to
accumulate all messages instead of replacing them: initialize errors["flags"] as
a list (or append to it if already present) and push each descriptive message
for the three checks, or concatenate messages with separators before finalizing;
ensure any downstream consumers that expect a string still receive a joined
string if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8854fa81-f26e-438d-8cdf-81c4ca860119
📒 Files selected for processing (30)
.ci-config/rippled.cfgCHANGELOG.mdtests/integration/reqs/test_account_objects_sponsored.pytests/integration/reqs/test_ledger_entry_sponsorship.pytests/integration/transactions/test_account_delete.pytests/integration/transactions/test_account_set.pytests/integration/transactions/test_sponsor_permissions.pytests/integration/transactions/test_sponsorship_set.pytests/integration/transactions/test_sponsorship_transfer.pytests/unit/models/transactions/test_better_transaction_flags.pytests/unit/models/transactions/test_payment.pytests/unit/models/transactions/test_sponsor_common_fields.pytests/unit/models/transactions/test_sponsor_permissions.pytests/unit/models/transactions/test_sponsor_signature.pytests/unit/models/transactions/test_sponsorship_set.pytests/unit/models/transactions/test_sponsorship_transfer.pyxrpl/asyncio/transaction/main.pyxrpl/core/binarycodec/definitions/definitions.jsonxrpl/core/binarycodec/definitions/definitions.pyxrpl/models/requests/account_objects.pyxrpl/models/requests/ledger_entry.pyxrpl/models/transactions/__init__.pyxrpl/models/transactions/account_set.pyxrpl/models/transactions/delegate_set.pyxrpl/models/transactions/payment.pyxrpl/models/transactions/sponsor_signature.pyxrpl/models/transactions/sponsorship_set.pyxrpl/models/transactions/sponsorship_transfer.pyxrpl/models/transactions/transaction.pyxrpl/models/transactions/types/transaction_type.py
| # SponsorSignature with multi-sign | ||
| # Fee = (1 + |tx.Signers| + |SponsorSignature.Signers|) × base | ||
| if transaction.sponsor is not None: | ||
| sponsor_signers_count = await _fetch_counterparty_signers_count( | ||
| client, transaction.sponsor | ||
| ) | ||
|
|
||
| if sponsor_signers_count > 1: | ||
| print( | ||
| ( | ||
| f"Warning: You are using autofill for a " | ||
| f"sponsored transaction: " | ||
| f"{transaction.to_dict()}. The fee " | ||
| "estimation is based on the number of " | ||
| "signers in the sponsor's SignerList. It " | ||
| "might be possible to optimize the fee by " | ||
| "considering the minimum quorum." | ||
| "\nIf you prefer optimized transaction fee," | ||
| " please fill the fee field manually." | ||
| ) | ||
| ) | ||
| base_fee += net_fee * (1 + sponsor_signers_count) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
XLS-68 XRPL sponsored fees transaction cost formula
💡 Result:
Under XLS-68 (Sponsored Fees & Reserves), the fee amount a transaction must pay is still the normal XRPL transaction fee; sponsorship only changes which balance is decremented (a sponsor-funded Sponsorship.FeeAmount pool, or the sponsor’s XRP balance), subject to constraints like MaxFee, reserve protection, and (optionally) requiring a sponsor signature. [1]
1) Baseline XRPL transaction cost (network/load-based)
From XRPL’s transaction-cost rules, the current minimum fee you generally target is:
Current Tx Cost (in XRP) = base_fee_xrp × load_factor [2]
(Equivalent values also exist in drops via server_state / fee APIs, but the key formula is the same scaling concept.) [2]
2) XLS-68 signature-related fee formula (when sponsorship adds sponsor multi-signing)
XLS-68 specifies that if sponsor multi-signatures are present/needed, the fee must cover added signature-processing overhead, and the fee calculation becomes:
Total fee = (1 + |tx.Signers| + |tx.SponsorSignature.Signers|) × base_fee + (transaction-specific fees) [1]
And specifically, if SponsorSignature.Signers is necessary, the minimum fee is:
Minimum fee = (|signatures| + 1) × base_fee [1]
3) Who actually pays (sponsored fees)
If a Sponsorship object exists, the ledger decrements tx.Fee from Sponsorship.FeeAmount; otherwise it decrements tx.Fee from the sponsor’s AccountRoot.Balance. [1]
Sources: [1] XLS-0068 spec (Sponsored Fees & Reserves). [2] XRPL.org “Transaction Cost”.
🏁 Script executed:
# Search for other references to sponsored fee calculation and sponsor_signers_count
rg -n "sponsor_signers_count|SponsorSignature" xrpl/asyncio/transaction/main.py -A 2 -B 2Repository: XRPLF/xrpl-py
Length of output: 677
🏁 Script executed:
# Look for tests related to sponsored fee calculation
fd -e py | xargs rg -l "sponsor.*fee\|SponsorSignature" | head -20Repository: XRPLF/xrpl-py
Length of output: 39
🏁 Script executed:
# Check if there's documentation about the fee calculation
fd -e md -e txt | xargs rg "XLS-68\|sponsor.*fee\|SponsorSignature" 2>/dev/null | head -20Repository: XRPLF/xrpl-py
Length of output: 39
🏁 Script executed:
# Look at the broader context around the sponsor fee calculation
cat -n xrpl/asyncio/transaction/main.py | sed -n '570,620p'Repository: XRPLF/xrpl-py
Length of output: 2601
🏁 Script executed:
# Get the exact indentation and structure of the counterparty and sponsor fee blocks
python3 << 'EOF'
with open('xrpl/asyncio/transaction/main.py', 'r') as f:
lines = f.readlines()
# Show lines 570-615 with visible whitespace
for i in range(569, 615):
line = lines[i]
# Replace spaces with dots and tabs with [TAB] for visibility
visible = line.rstrip('\n').replace(' ', '·').replace('\t', '[TAB]')
print(f"{i+1:3d}: {visible}")
EOFRepository: XRPLF/xrpl-py
Length of output: 2259
🏁 Script executed:
# Also search for how the counterparty fee is handled earlier in the function
rg -n "counterparty_signers_count" xrpl/asyncio/transaction/main.py -B 5 -A 5Repository: XRPLF/xrpl-py
Length of output: 1937
The sponsor fee is incorrectly omitted for single-signer sponsors.
The sponsor fee is only added when sponsor_signers_count > 1 (line 593), but should be calculated for all sponsors, even those with a single signer. This contradicts both the fee formula in the comment on line 587 and the XLS-68 specification, which states that the fee must include the sponsor's signer count regardless of whether it is 1, 2, or more.
Additionally, this behavior is inconsistent with the counterparty fee handling (lines 572–584), where the fee is added unconditionally outside the if counterparty_signers_count > 1: block, even for single-signer counterparties.
The fee calculation should be moved outside the conditional block and applied whenever transaction.sponsor is not None.
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 587-587: Comment contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xrpl/asyncio/transaction/main.py` around lines 586 - 607, The sponsor fee is
only added when sponsor_signers_count > 1, but per the comment/XLS‑68 it must be
applied whenever transaction.sponsor is not None; call
_fetch_counterparty_signers_count(client, transaction.sponsor) to get
sponsor_signers_count and always add base_fee += net_fee * (1 +
sponsor_signers_count) when transaction.sponsor is present, while keeping the
multi‑sign warning (the print) only inside the sponsor_signers_count > 1 branch;
update the block around transaction.sponsor, sponsor_signers_count, base_fee and
net_fee so the fee math is executed unconditionally for any sponsor.
There was a problem hiding this comment.
The fee calculation in the case of single-signer-sponsors works correctly. The rippled node does not reject this transaction for insufficient fee.
Look at the integration tests for SponsorshipSet and SponsorshipTransfer transactions in this PR. They pass the integ tests.
There was a problem hiding this comment.
@ckeshava, thank you for the clarification. That makes sense — for a single-signer sponsor, the SponsorSignature uses signing_pub_key + txn_signature fields rather than a Signers array, so |SponsorSignature.Signers| = 0 in the XLS-68 formula (1 + |tx.Signers| + |SponsorSignature.Signers|) × base_fee, meaning no additional fee beyond the base is required. The guard sponsor_signers_count > 1 is therefore correct, and the integration tests validate this. I'll retract my earlier concern.
(∩`-´)⊃━☆ﾟ.*･｡ﾟ
✏️ Learnings added
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 921
File: xrpl/asyncio/transaction/main.py:586-607
Timestamp: 2026-03-16T21:21:43.397Z
Learning: In xrpl/asyncio/transaction/main.py, for XLS-68 sponsored fee calculation, the additional fee for sponsor signatures is only required when the sponsor uses multi-signing (sponsor_signers_count > 1). For a single-signer sponsor, the SponsorSignature uses signing_pub_key + txn_signature fields (not a Signers array), so |SponsorSignature.Signers| = 0 in the XLS-68 formula (1 + |tx.Signers| + |SponsorSignature.Signers|) × base_fee, and no extra fee is needed. The guard `if sponsor_signers_count > 1` before `base_fee += net_fee * (1 + sponsor_signers_count)` is intentional and correct.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 757
File: xrpl/asyncio/transaction/main.py:505-509
Timestamp: 2025-06-05T20:52:31.099Z
Learning: The correct fee calculation for XRPL Batch transactions follows the C++ rippled implementation structure: (2 × base_fee) + sum of individual inner transaction fees + (signer_count × base_fee). Each inner transaction's fee should be calculated individually using the same fee calculation logic as standalone transactions, not assumed to be equal to base_fee.
Learnt from: ckeshava
Repo: XRPLF/xrpl-py PR: 759
File: xrpl/models/transactions/deposit_preauth.py:54-64
Timestamp: 2024-10-30T20:45:02.301Z
Learning: In the `DepositPreauth` class in `xrpl/models/transactions/deposit_preauth.py`, when validating that exactly one of the parameters `authorize`, `unauthorize`, `authorize_credentials`, or `unauthorize_credentials` is set, ensure that any refactoring maintains logical equivalence with the original condition to prevent altering the intended functionality.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/models/transactions/test_sponsorship_transfer.py (1)
174-174:⚠️ Potential issue | 🟡 MinorClarify REASSIGN docstring to avoid implying a
sponseerestriction.At Line 174, the phrase “(no sponsee)” suggests a rule that does not exist for
TF_SPONSORSHIP_REASSIGN; only CREATE has that restriction inxrpl/models/transactions/sponsorship_transfer.py_get_errors.📝 Suggested docstring fix
- """FlagInterface dict with TF_SPONSORSHIP_REASSIGN (no sponsee).""" + """FlagInterface dict with TF_SPONSORSHIP_REASSIGN."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/transactions/test_sponsorship_transfer.py` at line 174, Update the docstring that currently reads "FlagInterface dict with TF_SPONSORSHIP_REASSIGN (no sponsee)." to avoid implying a sponsee restriction; change the wording to simply reference TF_SPONSORSHIP_REASSIGN without "(no sponsee)". Ensure the docstring aligns with the validation logic in the _get_errors method (which enforces the sponsee restriction only for CREATE) and mention TF_SPONSORSHIP_REASSIGN and CREATE so reviewers can cross-check the behavior.
🧹 Nitpick comments (1)
tests/unit/models/transactions/test_sponsorship_transfer.py (1)
297-416: Reduce coupling to exact validation message text in invalid-input tests.These tests become brittle by asserting long message fragments; minor wording changes will fail tests without behavior changes. Prefer asserting exception type and a stable signal only.
♻️ Suggested pattern change
def test_invalid_end_and_create_flags(self): """Setting TF_SPONSORSHIP_END and TF_SPONSORSHIP_CREATE together is rejected.""" - with self.assertRaises(XRPLModelException) as cm: + with self.assertRaises(XRPLModelException): SponsorshipTransfer( account=_ACCOUNT, flags=( SponsorshipTransferFlag.TF_SPONSORSHIP_END | SponsorshipTransferFlag.TF_SPONSORSHIP_CREATE ), ) - self.assertIn(self._MULTI_FLAG_MSG, str(cm.exception))Based on learnings: In
tests/unit/models/transactions/test_credential_create.py, when testing invalid inputs, avoid adding assertions on specific error messages if it increases the complexity and maintenance overhead of the tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/transactions/test_sponsorship_transfer.py` around lines 297 - 416, The tests in test_sponsorship_transfer.py assert on long, exact validation message fragments which makes them brittle; update the tests (e.g., test_invalid_end_and_create_flags, test_invalid_end_and_reassign_flags, test_invalid_create_and_reassign_flags, test_invalid_sponsee_with_create_flag, test_invalid_sponsor_equals_account, test_invalid_sponsor_flags_without_sponsor, test_invalid_sponsor_signature_without_sponsor, test_invalid_sponsor_flags_bad_bits, test_invalid_sponsor_flags_combined_bad_bits) to stop asserting long message text on XRPLModelException from SponsorshipTransfer and instead either only assertRaises(XRPLModelException) or assert a minimal, stable signal (like the presence of a short token such as the field name "sponsor" or "sponsee") to verify the right validation path without coupling to exact wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/unit/models/transactions/test_sponsorship_transfer.py`:
- Line 174: Update the docstring that currently reads "FlagInterface dict with
TF_SPONSORSHIP_REASSIGN (no sponsee)." to avoid implying a sponsee restriction;
change the wording to simply reference TF_SPONSORSHIP_REASSIGN without "(no
sponsee)". Ensure the docstring aligns with the validation logic in the
_get_errors method (which enforces the sponsee restriction only for CREATE) and
mention TF_SPONSORSHIP_REASSIGN and CREATE so reviewers can cross-check the
behavior.
---
Nitpick comments:
In `@tests/unit/models/transactions/test_sponsorship_transfer.py`:
- Around line 297-416: The tests in test_sponsorship_transfer.py assert on long,
exact validation message fragments which makes them brittle; update the tests
(e.g., test_invalid_end_and_create_flags, test_invalid_end_and_reassign_flags,
test_invalid_create_and_reassign_flags, test_invalid_sponsee_with_create_flag,
test_invalid_sponsor_equals_account, test_invalid_sponsor_flags_without_sponsor,
test_invalid_sponsor_signature_without_sponsor,
test_invalid_sponsor_flags_bad_bits,
test_invalid_sponsor_flags_combined_bad_bits) to stop asserting long message
text on XRPLModelException from SponsorshipTransfer and instead either only
assertRaises(XRPLModelException) or assert a minimal, stable signal (like the
presence of a short token such as the field name "sponsor" or "sponsee") to
verify the right validation path without coupling to exact wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ecdf46ff-3449-41c7-8d3f-dfec7d583721
📒 Files selected for processing (1)
tests/unit/models/transactions/test_sponsorship_transfer.py
| "`sponsor_signature` requires `sponsor` to be set." | ||
| ) | ||
|
|
||
| # Pseudo-transactions and Batch cannot be sponsored (XLS-68 §8.3.4). |
There was a problem hiding this comment.
I like the comments referring to the exact section the edge case came from, very helpful
| # Late import to avoid circular dependency (sponsor_signature imports Signer from this | ||
| # module). This makes SponsorSignature available in the module namespace so that | ||
| # get_type_hints() can resolve the forward reference in Transaction.sponsor_signature. | ||
| from xrpl.models.transactions.sponsor_signature import ( # noqa: E402, F811 |
There was a problem hiding this comment.
Since this import is placed at the end of the file, I suspect it violates the linter configuration. However, this is necessary to avoid circular imports. noqa is used to disable the linter check on this line, it stands for No-Quality-Assurance
Explanation of noqa: https://stackoverflow.com/questions/45346575/what-does-noqa-mean-in-python-comments
| - New transaction types: `SponsorshipSet` and `SponsorshipTransfer` | ||
| - New ledger object type: `Sponsorship` (type code 144) | ||
| - New inner object model: `SponsorSignature` for sponsor co-signing |
There was a problem hiding this comment.
nit: I see the convention is to just list adding support, not any details for conciseness
achowdhry-ripple
left a comment
achowdhry-ripple
left a comment
There was a problem hiding this comment.
good PR -- my favorite part of AI here is the detailed commenting, it makes the code very interpretable and draws directly from the spec
…l-py into sponsoredFeesDraft1
…teg tests are also included to validate the sign_as_sponsor utility methods
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/integration/transactions/test_sponsor_signer.py (2)
161-161: Move import to top of file.The
decode_classic_addressimport on line 161 is inside the test method. For consistency with the rest of the codebase and to avoid repeated import overhead, move it to the top-level imports.♻️ Proposed fix
Add to the existing imports at the top:
from xrpl.asyncio.transaction import autofill, submit from xrpl.asyncio.transaction.main import sign +from xrpl.core.addresscodec import decode_classic_address from xrpl.models import PaymentThen remove line 161:
# Signers must be sorted by canonical account ID bytes (ascending). - from xrpl.core.addresscodec import decode_classic_address - ids = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/transactions/test_sponsor_signer.py` at line 161, The import of decode_classic_address is inside a test body; move the statement importing decode_classic_address from xrpl.core.addresscodec to the module's top-level imports (alongside the other imports in tests/integration/transactions/test_sponsor_signer.py) and remove the now-redundant inline import on line 161 so the test uses the top-level symbol instead.
60-60: Replace EN DASH with HYPHEN-MINUS in comments.Ruff flagged ambiguous EN DASH characters (
–) in step comments throughout the file. Replace them with standard HYPHEN-MINUS (-) for consistency (e.g.,# Step 1 -instead of# Step 1 –).Also applies to: 70-70, 76-76, 88-88, 123-123, 133-133, 137-137, 156-156, 169-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/transactions/test_sponsor_signer.py` at line 60, Replace ambiguous EN DASH characters (U+2013, "–") used in step comments with the standard HYPHEN-MINUS (U+002D, "-") to satisfy Ruff; specifically update comments like "# Step 1 – Sponsee builds and autofills the transaction." and the other occurrences indicated (the comments at the other steps such as the ones currently showing "–") so they read "# Step 1 - ..." (and likewise for Step 2, Step 3, etc.) in tests/integration/transactions/test_sponsor_signer.py, ensuring only the dash character in the comment text is changed.xrpl/transaction/sponsor_signer.py (2)
139-140: Unnecessary round-trip conversion.Lines 139-140 convert the transaction to a dict and back to a Transaction object, but
txis already a Transaction. This appears to be dead code that doesn't change anything.♻️ Proposed fix
- tx_dict = tx.to_dict() - tx = Transaction.from_dict(tx_dict) - tx_json = tx.to_xrpl()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xrpl/transaction/sponsor_signer.py` around lines 139 - 140, The two-line round-trip using tx.to_dict() and Transaction.from_dict(tx_dict) is unnecessary because tx is already a Transaction; remove the lines "tx_dict = tx.to_dict()" and "tx = Transaction.from_dict(tx_dict)" from sponsor_signer.py so the original tx object is used directly (unless there is a documented need to normalize or deep-copy, in which case replace with an explicit copy operation such as tx.copy() or a dedicated normalization function instead of the round-trip).
98-99: Replace EN DASH with HYPHEN-MINUS in docstrings.Ruff flagged ambiguous EN DASH characters (
–) in the docstrings. Replace them with standard HYPHEN-MINUS (-) for consistency.🔧 Proposed fix for lines 98-99
- - ``tx`` – the transaction with ``sponsor_signature`` added. - - ``tx_blob`` – the serialised transaction blob (no sponsee sig yet). + - ``tx`` - the transaction with ``sponsor_signature`` added. + - ``tx_blob`` - the serialised transaction blob (no sponsee sig yet).🔧 Proposed fix for lines 212-213
- - ``tx`` – the combined transaction object. - - ``tx_blob`` – the serialised hex blob ready for the sponsee to sign + - ``tx`` - the combined transaction object. + - ``tx_blob`` - the serialised hex blob ready for the sponsee to signAlso applies to: 212-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xrpl/transaction/sponsor_signer.py` around lines 98 - 99, Replace the ambiguous EN DASH characters (U+2013) used in docstring bullet points with the standard ASCII HYPHEN-MINUS ('-') in xrpl/transaction/sponsor_signer.py; specifically update the bullets describing ``tx`` and ``tx_blob`` (the lines referring to those symbols) and the other docstring occurrence near the 212-213 area so all hyphens are ASCII hyphen-minus characters, then run the linter to ensure no other EN DASH instances remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xrpl/transaction/sponsor_signer.py`:
- Around line 261-265: The function _strip_signers currently uses a bare dict
return type; update its annotation to return Dict[str, Any] (i.e., def
_strip_signers(tx: Transaction) -> Dict[str, Any]:) and add the required typing
imports (from typing import Any, Dict) at the top of the file so mypy no longer
reports "Missing type parameters for generic type 'dict'"; keep the function
body unchanged aside from the new return type annotation.
---
Nitpick comments:
In `@tests/integration/transactions/test_sponsor_signer.py`:
- Line 161: The import of decode_classic_address is inside a test body; move the
statement importing decode_classic_address from xrpl.core.addresscodec to the
module's top-level imports (alongside the other imports in
tests/integration/transactions/test_sponsor_signer.py) and remove the
now-redundant inline import on line 161 so the test uses the top-level symbol
instead.
- Line 60: Replace ambiguous EN DASH characters (U+2013, "–") used in step
comments with the standard HYPHEN-MINUS (U+002D, "-") to satisfy Ruff;
specifically update comments like "# Step 1 – Sponsee builds and autofills the
transaction." and the other occurrences indicated (the comments at the other
steps such as the ones currently showing "–") so they read "# Step 1 - ..." (and
likewise for Step 2, Step 3, etc.) in
tests/integration/transactions/test_sponsor_signer.py, ensuring only the dash
character in the comment text is changed.
In `@xrpl/transaction/sponsor_signer.py`:
- Around line 139-140: The two-line round-trip using tx.to_dict() and
Transaction.from_dict(tx_dict) is unnecessary because tx is already a
Transaction; remove the lines "tx_dict = tx.to_dict()" and "tx =
Transaction.from_dict(tx_dict)" from sponsor_signer.py so the original tx object
is used directly (unless there is a documented need to normalize or deep-copy,
in which case replace with an explicit copy operation such as tx.copy() or a
dedicated normalization function instead of the round-trip).
- Around line 98-99: Replace the ambiguous EN DASH characters (U+2013) used in
docstring bullet points with the standard ASCII HYPHEN-MINUS ('-') in
xrpl/transaction/sponsor_signer.py; specifically update the bullets describing
``tx`` and ``tx_blob`` (the lines referring to those symbols) and the other
docstring occurrence near the 212-213 area so all hyphens are ASCII hyphen-minus
characters, then run the linter to ensure no other EN DASH instances remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22999a35-d3bf-4309-8d32-45f702800941
📒 Files selected for processing (3)
tests/integration/transactions/test_sponsor_signer.pyxrpl/transaction/__init__.pyxrpl/transaction/sponsor_signer.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xrpl/transaction/sponsor_signer.py`:
- Around line 98-99: Docstring uses Unicode en dashes for the bullet lines
describing ``tx`` and ``tx_blob``; replace each EN DASH (U+2013) with a standard
ASCII hyphen-minus (U+002D) so Ruff RUF002 is satisfied. Locate the
return/descriptive section in xrpl/transaction/sponsor_signer.py that lists
``tx`` – the transaction... and ``tx_blob`` – the serialised..., and change the
dash characters to "-"; apply the same replacement to the other occurrences
noted around the section that also lists those items (the other instance at the
later docstring lines mentioned).
- Line 26: The module docstring contains a malformed Sphinx function reference
`:func:\`combine_sponsor_signers\`\``; open the module docstring in
sponsor_signer.py and replace that token with a proper Sphinx role
`:func:`combine_sponsor_signers`` (i.e., remove the extra backtick) so the
reference to the combine_sponsor_signers function renders correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59236108-2582-4ff2-b17e-0cfdfed5c1b7
📒 Files selected for processing (1)
xrpl/transaction/sponsor_signer.py
|
|
||
| For sponsor accounts that require multiple keys (multi-sig), each key holder | ||
| calls :func:`sign_as_sponsor` with ``multisign=True``, then all contributions | ||
| are merged with :func:`combine_sponsor_signers`` before the sponsee signs. |
There was a problem hiding this comment.
Fix malformed Sphinx function reference in the module docstring.
There’s an extra backtick in :func:\combine_sponsor_signers``` which can break docs rendering.
Suggested fix
-calls :func:`sign_as_sponsor` with ``multisign=True``, then all contributions
-are merged with :func:`combine_sponsor_signers`` before the sponsee signs.
+calls :func:`sign_as_sponsor` with ``multisign=True``, then all contributions
+are merged with :func:`combine_sponsor_signers` before the sponsee signs.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| are merged with :func:`combine_sponsor_signers`` before the sponsee signs. | |
| are merged with :func:`combine_sponsor_signers` before the sponsee signs. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xrpl/transaction/sponsor_signer.py` at line 26, The module docstring contains
a malformed Sphinx function reference `:func:\`combine_sponsor_signers\`\``;
open the module docstring in sponsor_signer.py and replace that token with a
proper Sphinx role `:func:`combine_sponsor_signers`` (i.e., remove the extra
backtick) so the reference to the combine_sponsor_signers function renders
correctly.
| - ``tx`` – the transaction with ``sponsor_signature`` added. | ||
| - ``tx_blob`` – the serialised transaction blob (no sponsee sig yet). |
There was a problem hiding this comment.
Replace EN DASH with standard hyphen to satisfy Ruff (RUF002).
These lines currently use ambiguous Unicode dashes.
Suggested fix
- - ``tx`` – the transaction with ``sponsor_signature`` added.
- - ``tx_blob`` – the serialised transaction blob (no sponsee sig yet).
+ - ``tx`` - the transaction with ``sponsor_signature`` added.
+ - ``tx_blob`` - the serialised transaction blob (no sponsee sig yet).
...
- - ``tx`` – the combined transaction object.
- - ``tx_blob`` – the serialised hex blob ready for the sponsee to sign
+ - ``tx`` - the combined transaction object.
+ - ``tx_blob`` - the serialised hex blob ready for the sponsee to signAlso applies to: 212-213
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 98-98: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
[warning] 99-99: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xrpl/transaction/sponsor_signer.py` around lines 98 - 99, Docstring uses
Unicode en dashes for the bullet lines describing ``tx`` and ``tx_blob``;
replace each EN DASH (U+2013) with a standard ASCII hyphen-minus (U+002D) so
Ruff RUF002 is satisfied. Locate the return/descriptive section in
xrpl/transaction/sponsor_signer.py that lists ``tx`` – the transaction... and
``tx_blob`` – the serialised..., and change the dash characters to "-"; apply
the same replacement to the other occurrences noted around the section that also
lists those items (the other instance at the later docstring lines mentioned).
3 participants
High Level Overview of Change
This PR provides support for SponsoredFeesAndReserves amendment in xrpl-py library.
C++ Feature branch: XRPLF/rippled#5887
XLS specification: https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0068-sponsored-fees-and-reserves
XLS spec proposed updates: https://github.com/XRPLF/XRPL-Standards/pull/465/changes
If you would like to run the integ tests, please build the C++ feature branch linked above.
Context of Change
xrpl-py does not have ledger-object modelsaccount_sponsoringRPC is expected to be implemented in Clio. I will include it in this implementation once Clio feature is ready.did not find a need for new util methodsType of Change
Did you update CHANGELOG.md?
Test Plan
Appropriate tests have been added