feat: add split payments support for Tempo charges#104
feat: add split payments support for Tempo charges#104brendanjryan wants to merge 6 commits intomainfrom
Conversation
Port of mpp-rs PR #180. Allows a single charge to be split across multiple recipients. Changes: - Split model and splits field on MethodDetails (schemas) - get_transfers() computes ordered transfer list (primary + splits) - Multi-transfer verification in receipt logs and transaction calldata - Order-insensitive matching with memo-specificity sorting - Client builds multiple Call objects when splits present - Server charge() accepts splits parameter - Scope binding: memo and splits compared in verify flow
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc13896b9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/mpp/methods/tempo/client.py
Outdated
| td = self._encode_transfer(t.recipient, t.amount) | ||
| call_list.append(Call.create(to=currency, value=0, data=td)) | ||
| calls_tuple = tuple(call_list) | ||
| gas_estimate_data = call_list[0].data.hex() if call_list else None |
There was a problem hiding this comment.
In the split-payment branch, gas_estimate_data is taken from call_list[0].data.hex(), which drops the 0x prefix before passing it to estimate_gas(). estimate_gas() forwards that value directly to eth_estimateGas, so split transactions send malformed calldata for estimation and hit the broad except, forcing DEFAULT_GAS_LIMIT every time. This can cause valid split payments to fail at broadcast/receipt time when multi-call gas usage exceeds the static fallback.
Useful? React with 👍 / 👎.
src/mpp/methods/tempo/intents.py
Outdated
| b = bytes.fromhex(hex_str) | ||
| except ValueError: | ||
| return None | ||
| return b if len(b) == 32 else None |
There was a problem hiding this comment.
_parse_memo_bytes() returns None for invalid hex or wrong-length memo strings, and get_transfers() uses that value directly. For split charges, this silently downgrades an invalid requested memo to “no memo” instead of rejecting the request, so both client construction and server verification can accept transfers without the intended memo binding. This is a fail-open behavior for malformed memo inputs.
Useful? React with 👍 / 👎.
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This PR ports split payment support from mpp-rs PR #180 into the Python SDK. The core split implementation is structurally sound and well-tested. However, the _parse_memo_bytes refactoring introduces a fail-open regression where invalid explicit memos are silently downgraded to None, weakening payment uniqueness enforcement across receipt matching, calldata validation, and fee-payer preflight. Additionally, memo-less split transfers permissively accept transferWithMemo calls/logs without checking memo contents, enabling cross-intent double spending. Several compatibility and gas-estimation issues also need attention.
Reviewer Callouts
- ⚡
get_transfers()is now security-critical: This shared helper feeds receipt matching, fee-payer cosigning, and pre-broadcast payload validation simultaneously. Any future fail-open parsing bug here silently weakens all three validation boundaries. - ⚡ Transaction-credential replay store gap (pre-existing):
_verify_transaction()never writestx_hashto the replay store after broadcast; only_verify_hash()records hashes. Split receipts widen this gap by creating multiple independently-matchable transfer legs. Consider recordingtx_hashafter transaction verification to match upstream Rust behavior. - ⚡ Pre-existing extra-call piggybacking:
_validate_calls()only checks that expected transfers are a subset of provided calls — extra arbitrary calls pass validation, enabling sponsored gas abuse. - ⚡ Mixed-SDK compatibility: Enabling splits on a Python server will break Rust/non-Python clients that still construct single-transfer payments.
src/mpp/methods/tempo/intents.py
Outdated
| try: | ||
| b = bytes.fromhex(hex_str) | ||
| except ValueError: | ||
| return None |
There was a problem hiding this comment.
🚨 [SECURITY] _parse_memo_bytes silent failure downgrades explicit memos to wildcard matching
When a merchant supplies a memo that isn't exactly 32 bytes of hex (e.g., short order IDs, unpadded hex), this returns None instead of raising an error. That None propagates through get_transfers() into all verification paths, causing the server to accept any same-amount transfer regardless of memo — breaking payment uniqueness.
Previously, non-standard memos caused fail-closed behavior. The upstream Rust server rejects invalid memos with a VerificationError rather than silently downgrading.
Recommended Fix: Raise VerificationError when a memo string is explicitly provided but cannot be parsed as 32 bytes of hex. Reserve None return exclusively for when no memo was supplied.
src/mpp/methods/tempo/intents.py
Outdated
| if memo is not None: | ||
| if selector != TRANSFER_WITH_MEMO_SELECTOR: | ||
| return False | ||
| elif selector not in (TRANSFER_SELECTOR, TRANSFER_WITH_MEMO_SELECTOR): |
There was a problem hiding this comment.
🚨 [SECURITY] Memo-less transfers accept transferWithMemo calls, enabling cross-intent double spending
When memo is None, this accepts both TRANSFER_SELECTOR and TRANSFER_WITH_MEMO_SELECTOR without inspecting memo bytes. An attacker can craft a transferWithMemo call carrying a memo that binds to a different payment intent, allowing one on-chain call to satisfy both a memo-less split here and a memo-bound primary transfer elsewhere — a double-spending vector.
The same permissive check exists in _verify_transfer_logs() at the multi-transfer branch (line ~504).
Recommended Fix: When memo is None, strictly require TRANSFER_SELECTOR only. Reject transferWithMemo for memo-less expected transfers.
src/mpp/methods/tempo/client.py
Outdated
| td = self._encode_transfer(t.recipient, t.amount) | ||
| call_list.append(Call.create(to=currency, value=0, data=td)) | ||
| calls_tuple = tuple(call_list) | ||
| gas_estimate_data = call_list[0].data.hex() if call_list else None |
There was a problem hiding this comment.
gas_estimate_data is derived from call_list[0].data.hex() only, but get_transfers() can produce up to 11 calls (1 primary + 10 splits). The DEFAULT_GAS_LIMIT = 1_000_000 floor mitigates small split counts, but large batches with fresh recipients could exceed that floor on Tempo. In sponsored mode, reverting transactions burn gas at the sponsor's expense.
Recommended Fix: Estimate gas from the full call list, or use a per-transfer upper bound that scales with split count.
| if memo: | ||
| method_details["memo"] = memo | ||
| if splits: | ||
| method_details["splits"] = splits |
There was a problem hiding this comment.
splits is serialized into methodDetails and fee_payer=True is supported, but fee_payer_url is not exposed. The convenience path falls back to DEFAULT_FEE_PAYER_URL (hosted Rust sponsor), which has no splits field in TempoMethodDetails and validates only a single full-amount transfer. Split-sponsored transactions will silently fail.
Recommended Fix: Gate or reject splits + fee_payer=True until a split-aware sponsor is available, or plumb fee_payer_url through the API.
💡 [SUGGESTION] Split amounts should use the same unit semantics as amount
Mpp.charge() normalizes top-level amount via parse_units() but forwards splits[*].amount unchanged. Downstream code consumes them as raw base-unit integers. Callers passing human-readable amounts like "0.30" will get ValueError. Consider applying the same decimals-based normalization.
- _parse_memo_bytes: raise VerificationError on invalid explicit memos instead of silently downgrading to None (fail-closed) - Memo-less transfers strictly require TRANSFER_SELECTOR only, rejecting transferWithMemo to prevent cross-intent double spending - Gas estimation sums estimates across all calls in split batches instead of using only the first call - Reject splits + fee_payer=True until a split-aware sponsor is available
- _parse_memo_bytes: valid input, invalid hex, wrong length, empty - _match_single_transfer_calldata: memo strictness, no-memo rejection - Log verification: memo-less single/multi rejects TransferWithMemo - splits + fee_payer: raises ValueError - get_transfers: invalid/short memos on primary and split
2 participants
Summary
Port of mpp-rs PR #180 — adds split payments to Tempo charges, allowing a single charge to be split across multiple recipients.