feat!: adds payref to ootle address - #1629
Conversation
WalkthroughExtends the Tari wallet to support optional payment references (PayRef) embedded in addresses and memos. Changes include new Memo variants (U256 and PayRefAndBytes), OotleAddress serialization/deserialization to carry PayRef payloads, updated TypeScript bindings with PayRef type and address encoding helpers, new wallet daemon RPC endpoint for PayRef address generation, and web UI components for displaying and managing PayRef data through dialogs and QR codes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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: 7
🧹 Nitpick comments (7)
crates/template_lib_types/src/error.rs (1)
14-16: LGTM! Clean constructor implementation.The constructor correctly initializes the error fields and follows idiomatic Rust patterns. Good use of
pub(crate)fields with a public constructor for proper encapsulation.Minor observation: Consider adding a getter method for the
expectedfield (similar toactual_size()for thesizefield) to provide a consistent API for accessing error details.applications/tari_walletd/src/handlers/accounts.rs (1)
1001-1002: Consider warning users when memo content is truncated.The function
new_pay_ref_and_bytes_truncatesilently truncates the message bytes to fit alongside the pay_ref. Users may not realize their message was shortened, potentially leading to confusion or data loss.Consider adding a log warning when truncation occurs:
+ let original_len = memo_bytes.len(); let memo = Memo::new_pay_ref_and_bytes_truncate(pay_ref, memo_bytes) .ok_or_else(|| invalid_params("pay_ref", Some("pay_ref is too large to fit in memo")))?; + if memo.as_memo_bytes().map(|b| b.len()).unwrap_or(0) < original_len { + warn!( + target: LOG_TARGET, + "⚠️ Memo content was truncated to fit pay_ref for transfer to address {}", + transfer.destination_address + ); + }applications/tari_walletd/web_ui/src/components/CopyAddress.tsx (1)
35-35: Potentially dead code: typeof object check after type narrowing.Line 35 checks
typeof display === "object", butdisplayis now typed asstring(line 29), making this branch unreachable. Consider removing the object check or clarifying the intent.Apply this diff to simplify the logic:
- const displayStr = display && (typeof display === "object" ? shortenSubstateId(display) : display); + const displayStr = display;bindings/src/types/Memo.ts (1)
3-3: Update Memo component to explicitly handle U256 variant for consistent UX.The PayRefAndBytes variant is properly handled. However, U256 currently falls through to the fallback
JSON.stringify()case, which renders as{"U256":"..."}rather than a user-friendly display. Add explicit handling for U256 similar to the Bytes variant:if ("U256" in memo) { return <span>{memo ? memo.U256 : "No Memo"}</span>; }Place this before the final fallback in
applications/tari_walletd/web_ui/src/components/Memo.tsx.applications/tari_walletd/web_ui/src/components/Memo.tsx (1)
6-6: Unused import detected.The
CopyAddressimport on line 6 doesn't appear to be used in this file.Apply this diff to remove the unused import:
-import CopyAddress from "@components/CopyAddress";applications/tari_walletd/web_ui/src/routes/AssetVault/Components/AccountDetails.tsx (1)
132-132: Add input validation and meaningful form handling.Line 132 has an empty
onSubmithandler. If form submission isn't needed, consider removing the<form>wrapper or providing a meaningful handler (e.g., copying the address or closing the dialog).Additionally, the payment reference TextField (lines 146-152) has no validation. Consider adding constraints to prevent excessively long payment references that could exceed encoding limits.
Apply this diff to add basic validation:
const handleOnChange = (event: React.ChangeEvent<HTMLInputElement>) => { + const payRefValue = event.target.value; + // Optionally add length validation here + if (payRefValue.length > 100) { // Adjust limit as needed + return; + } const decoded = decodeOotleAddress(address); - decoded.payRef = event.target.value; + decoded.payRef = payRefValue; const addr = encodeOotleAddress(decoded); setCurrentAddress(addr); };Also applies to: 146-152
applications/tari_walletd/web_ui/src/components/TransactionsStatusChip.tsx (1)
70-74: Remove unused variables.Lines 71-72 declare
leftColorandrightColorbut these variables are never used in the code path whenshowTitleis false. These should be removed to clean up the code.Apply this diff to remove the unused variables:
if (!showTitle) { - let leftColor = colorList["Accepted"]; - let rightColor = colorList["Rejected"]; - return <Avatar sx={{ bgcolor: bgColor, height: 22, width: 22 }}>{iconList[status]}</Avatar>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
applications/tari_walletd/src/handlers/accounts.rs(1 hunks)applications/tari_walletd/web_ui/package.json(1 hunks)applications/tari_walletd/web_ui/src/components/CopyAddress.tsx(1 hunks)applications/tari_walletd/web_ui/src/components/Memo.tsx(2 hunks)applications/tari_walletd/web_ui/src/components/StatusChip.tsx(1 hunks)applications/tari_walletd/web_ui/src/components/TransactionsStatusChip.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Components/AccountDetails.tsx(4 hunks)applications/tari_walletd/web_ui/src/routes/Transactions/TransactionDetails.tsx(2 hunks)applications/tari_walletd/web_ui/src/routes/Transactions/Transactions.tsx(2 hunks)bindings/package.json(2 hunks)bindings/src/helpers/ootleAddress.ts(5 hunks)bindings/src/index.ts(1 hunks)bindings/src/types/Memo.ts(1 hunks)bindings/src/types/PayRef.ts(1 hunks)bindings/test/ootleAddress.test.ts(4 hunks)clients/javascript/wallet_daemon_client/package.json(1 hunks)clients/javascript/wallet_daemon_client/src/index.ts(2 hunks)clients/wallet_daemon_client/Cargo.toml(1 hunks)crates/ootle_address/src/lib.rs(1 hunks)crates/ootle_address/src/ootle_address.rs(18 hunks)crates/ootle_address/src/pay_ref.rs(1 hunks)crates/template_lib_types/src/error.rs(1 hunks)crates/wallet/crypto/src/encrypted_data.rs(1 hunks)crates/wallet/crypto/src/memo.rs(9 hunks)crates/wallet/sdk/src/apis/key_manager.rs(1 hunks)crates/wallet/sdk/tests/base_layer_compat.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
crates/ootle_address/src/lib.rs (1)
crates/ootle_address/src/ootle_address.rs (2)
pay_ref(74-76)pay_ref(364-366)
bindings/src/types/Memo.ts (1)
applications/tari_walletd/web_ui/src/components/Memo.tsx (1)
Memo(13-49)
applications/tari_walletd/web_ui/src/routes/Transactions/TransactionDetails.tsx (1)
applications/tari_walletd/web_ui/src/components/TransactionsStatusChip.tsx (1)
TransactionsStatusChip(43-85)
applications/tari_walletd/web_ui/src/routes/Transactions/Transactions.tsx (1)
applications/tari_walletd/web_ui/src/components/TransactionsStatusChip.tsx (1)
TransactionsStatusChip(43-85)
applications/tari_walletd/web_ui/src/components/Memo.tsx (1)
applications/tari_walletd/web_ui/src/components/StatusChip.tsx (2)
StatusChip(55-94)StatusChipColors(37-44)
crates/wallet/sdk/tests/base_layer_compat.rs (2)
crates/wallet/crypto/src/memo.rs (2)
new_u256(54-57)decode_from(197-241)bindings/src/types/Memo.ts (1)
Memo(3-3)
applications/tari_walletd/src/handlers/accounts.rs (4)
crates/ootle_address/src/ootle_address.rs (2)
pay_ref(74-76)pay_ref(364-366)crates/wallet/crypto/src/memo.rs (2)
new_message(59-63)new_pay_ref_and_bytes_truncate(81-91)applications/tari_walletd/src/handlers/helpers.rs (1)
invalid_params(162-173)bindings/src/types/wallet-daemon-client/TransferOutput.ts (1)
TransferOutput(6-24)
applications/tari_walletd/web_ui/src/components/TransactionsStatusChip.tsx (1)
bindings/src/types/TransactionStatus.ts (1)
TransactionStatus(3-11)
bindings/src/helpers/ootleAddress.ts (1)
bindings/src/types/Network.ts (1)
Network(6-6)
crates/ootle_address/src/pay_ref.rs (4)
bindings/src/types/PayRef.ts (1)
PayRef(3-3)crates/wallet/crypto/src/memo.rs (1)
len(108-115)crates/ootle_address/src/ootle_address.rs (5)
pay_ref(74-76)pay_ref(364-366)serialize(300-310)new(31-38)new(338-345)crates/template_lib_types/src/hex.rs (2)
bytes_from_hex(20-31)bytes_to_hex(33-37)
applications/tari_walletd/web_ui/src/routes/AssetVault/Components/AccountDetails.tsx (2)
bindings/src/types/OotleAddress.ts (1)
OotleAddress(3-3)bindings/src/helpers/ootleAddress.ts (2)
decodeOotleAddress(37-90)encodeOotleAddress(92-135)
crates/wallet/crypto/src/memo.rs (5)
crates/wallet/crypto/src/unblinded_statement.rs (2)
value(60-62)memo(68-70)applications/tari_walletd/web_ui/src/components/Memo.tsx (1)
Memo(13-49)bindings/src/types/Memo.ts (1)
Memo(3-3)crates/template_lib_types/src/max_bytes.rs (1)
new_checked(26-33)crates/template_lib_types/src/max_string.rs (1)
new_checked(20-27)
bindings/test/ootleAddress.test.ts (1)
bindings/src/helpers/ootleAddress.ts (3)
encodeOotleAddress(92-135)DecodedOotleAddress(12-17)decodeOotleAddress(37-90)
crates/ootle_address/src/ootle_address.rs (2)
crates/ootle_address/src/hrp.rs (2)
hrp_from_network(14-23)network_from_hrp(25-41)crates/ootle_address/src/pay_ref.rs (3)
from_bytes(33-35)len(21-23)new_checked(12-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check nightly
- GitHub Check: clippy
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: machete
- GitHub Check: fmt
🔇 Additional comments (22)
bindings/package.json (2)
3-4: Version bump and package metadata look good.The minor version bump to 1.20.1 is appropriate for adding new PayRef types and encoding/decoding helpers to the bindings. The description field provides useful context for consumers of the NPM package.
21-21: Minor: Build script whitespace cleanup.Removed the extra space between
pnpm run fmt &&andtsc. This is a nice formatting improvement with no functional impact.clients/javascript/wallet_daemon_client/package.json (1)
3-3: Verify version bump strategy aligns with breaking-change status.The PR title indicates a breaking change ("feat!"), but the version is bumped from 1.11.0 to 1.11.1 (patch level). Per semantic versioning:
- Patch bumps (1.11.1) are used for backward-compatible bug fixes or non-breaking changes.
- Major bumps (2.0.0) are required for breaking changes.
However, the PR objectives state no breaking changes are included. There's a contradiction between the PR title ("feat!") and the stated breaking-change status. If the feature is truly non-breaking, the version should be 1.12.0 (minor bump) to reflect the new PayRef functionality. If it is breaking, the version should be 2.0.0.
Please clarify:
- Are there actual breaking changes in this PR, or should the commit title be "feat:" (without the "!")?
- Should the version be 1.12.0 (minor bump for new feature) or 2.0.0 (major bump for breaking changes)?
clients/wallet_daemon_client/Cargo.toml (1)
30-30: Change verified and approved.All three dependent crates properly define the
tsfeature:
tari_ootle_wallet_sdk:ts = ["ts-rs", "tari_ootle_address/ts"]tari_ootle_common_types:ts = ["ts-rs"]tari_ootle_address:ts = ["ts-rs"]The feature propagation in the file is syntactically correct and all referenced features exist.
clients/javascript/wallet_daemon_client/src/index.ts (2)
20-20: LGTM! Import additions are clean.The new types
AccountsGetPayRefAddressRequestandAccountsGetPayRefAddressResponseare properly imported and grouped logically with related account types.
180-182: Code review verified and approved.The types
AccountsGetPayRefAddressRequestandAccountsGetPayRefAddressResponseare correctly imported from@tari-project/typescript-bindings(line 15 of imports). The method implementation follows the established pattern used by all other RPC wrapper methods in the class, with consistent naming conventions for both the method name (camelCase) and RPC endpoint (snake_case). No issues found.applications/tari_walletd/src/handlers/accounts.rs (1)
981-1026: Good refactoring to pre-process outputs.The change to collect and transform outputs before passing them to
StealthTransferParamsis a solid improvement. Pre-processing outputs makes the pay_ref embedding logic clearer and keeps the params construction clean.crates/wallet/crypto/src/encrypted_data.rs (1)
219-219: LGTM - Minor grammatical improvement.The comment update improves readability without any functional changes.
applications/tari_walletd/web_ui/package.json (1)
28-28: LGTM - QR code dependency added for PayRef feature.The addition of
react-qr-codesupports the new PayRef UI functionality for displaying addresses as QR codes.crates/ootle_address/src/pay_ref.rs (2)
60-84: LGTM - Serde implementation correctly validates PayRef constraints.The serialization uses hex for human-readable formats and raw bytes otherwise. Deserialization properly validates length constraints through
new_checked, preventing invalid PayRef instances.
12-19: Verify PayRef validation aligns with consumer expectations.The validation requires PayRef to be non-empty and ≤ 64 bytes. Ensure this constraint is documented and enforced consistently across all creation paths (including deserialization and UI inputs).
Run the following script to check for any PayRef creation that might bypass validation:
bindings/src/index.ts (1)
84-84: LGTM - PayRef type properly exported.The export follows the existing pattern and makes the PayRef type available to TypeScript consumers.
crates/wallet/sdk/src/apis/key_manager.rs (1)
195-211: LGTM - PayRef correctly initialized as None for default addresses.The
pay_ref: Noneinitialization is appropriate for thederive_account_addressmethod, which generates standard addresses without payment references. This aligns with the optional PayRef support in OotleAddress.crates/ootle_address/src/lib.rs (1)
6-9: LGTM - PayRef module properly declared and exported.The module declaration and re-export follow the existing pattern and make PayRef publicly available from the crate root.
applications/tari_walletd/web_ui/src/routes/Transactions/TransactionDetails.tsx (1)
51-51: LGTM! Component refactor is consistent.The replacement of
StatusChipwithTransactionsStatusChipis clean and maintains the same props interface.Also applies to: 179-179
bindings/src/types/PayRef.ts (1)
1-3: LGTM! Simple and appropriate type definition.The
PayReftype alias as a string provides flexibility for payment reference formats.applications/tari_walletd/web_ui/src/components/Memo.tsx (1)
51-58: LGTM! Clean helper function.The
tryDecodeUtf8helper provides a safe way to attempt UTF-8 decoding with appropriate fallback handling.bindings/test/ootleAddress.test.ts (1)
8-14: LGTM! Test improvements enhance coverage.The helper rename to
makeDecodedAddressis more descriptive, and the new test case provides thorough coverage of the payRef encoding/decoding functionality, including verification that addresses with and without payRef differ.Also applies to: 31-50
crates/wallet/sdk/tests/base_layer_compat.rs (1)
20-27: LGTM! Solid test coverage for U256 memos.The new test path correctly verifies round-trip serialization and deserialization of 256-bit memo fields, ensuring compatibility between
MemoField::new_u256andMemo::new_u256.applications/tari_walletd/web_ui/src/routes/Transactions/Transactions.tsx (1)
40-40: LGTM! Consistent component refactor.The replacement of
StatusChipwithTransactionsStatusChipmaintains the same props interface (statusandshowTitle) and is consistent with the refactor in TransactionDetails.tsx.Also applies to: 103-103
applications/tari_walletd/web_ui/src/routes/AssetVault/Components/AccountDetails.tsx (1)
56-56: LGTM! Clean dialog integration.The PayRef dialog state management and trigger button are well-implemented with appropriate tooltips for user guidance.
Also applies to: 69-69, 92-96
applications/tari_walletd/web_ui/src/components/TransactionsStatusChip.tsx (1)
43-84: LGTM! Well-structured status chip component.The
TransactionsStatusChipcomponent provides clear visual feedback for transaction statuses with appropriate icons and colors. The special handling forOnlyFeeAcceptedwith a gradient background is a nice touch for representing the mixed state.
| let memo = Memo::new_pay_ref_and_bytes_truncate(pay_ref, memo_bytes) | ||
| .expect("payref + truncated message fits in memo"); |
There was a problem hiding this comment.
Handle potential None from new_pay_ref_and_bytes_truncate gracefully.
The expect() on line 1002 will panic if new_pay_ref_and_bytes_truncate returns None. According to the implementation in memo.rs, this function returns None when the pay_ref itself is too large to fit in a memo (exceeds MAX_BYTES_LENGTH - 1). This should be handled with a proper error instead of panicking.
Apply this diff to handle the error gracefully:
- let memo = Memo::new_pay_ref_and_bytes_truncate(pay_ref, memo_bytes)
- .expect("payref + truncated message fits in memo");
+ let memo = Memo::new_pay_ref_and_bytes_truncate(pay_ref, memo_bytes)
+ .ok_or_else(|| invalid_params("pay_ref", Some("pay_ref is too large to fit in memo")))?;📝 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.
| let memo = Memo::new_pay_ref_and_bytes_truncate(pay_ref, memo_bytes) | |
| .expect("payref + truncated message fits in memo"); | |
| let memo = Memo::new_pay_ref_and_bytes_truncate(pay_ref, memo_bytes) | |
| .ok_or_else(|| invalid_params("pay_ref", Some("pay_ref is too large to fit in memo")))?; |
🤖 Prompt for AI Agents
In applications/tari_walletd/src/handlers/accounts.rs around lines 1001-1002,
replace the unconditional expect on Memo::new_pay_ref_and_bytes_truncate (which
can return None when pay_ref is too large) with a graceful check that handles
the None case and returns an appropriate error instead of panicking; detect the
None result, construct and return a meaningful error (e.g., a
BadRequest/InvalidArgument or domain-specific error indicating the pay_ref is
too large to fit in a memo), or propagate it via the function's Result, ensuring
no panic path remains.
There was a problem hiding this comment.
Because payref has maximum of 64 bytes and the rest of truncated, returning None is not possible (or if it does, it's a bug)
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the implementation of new_pay_ref_and_bytes_truncate in memo.rs
echo "=== Checking new_pay_ref_and_bytes_truncate implementation ==="
ast-grep --pattern $'fn new_pay_ref_and_bytes_truncate($$$) -> Option<Self> {
$$$
}'
echo -e "\n=== Checking MAX_BYTES_LENGTH constant in memo.rs ==="
rg -n "MAX_BYTES_LENGTH" crates/wallet/crypto/src/memo.rs -A 2 -B 2
echo -e "\n=== Checking PayRef::MAX_LEN ==="
rg -n "MAX_LEN" crates/ootle_address/src/pay_ref.rs -A 1 -B 1
Length of output: 4948
@sdbondi You're absolutely correct! I've verified the implementation and your reasoning is sound:
PayRef::MAX_LEN= 64 bytes (enforced inPayRef::new_checked)Memo::MAX_BYTES_LENGTH= 253 bytesnew_pay_ref_and_bytes_truncateonly returnsNoneifpay_ref.len() > MAX_BYTES_LENGTH - 1(i.e., > 252 bytes)- Since
64 < 252, a validPayRefwill never cause this function to returnNone
The expect() is safe here because the payref is validated during address decoding, ensuring it never exceeds 64 bytes. The message portion is truncated as needed to fit in the remaining space (253 - 1 byte length prefix - payref length).
The only minor note is that the expect message "payref + truncated message fits in memo" is slightly imprecise—it's really about the payref size being valid, but that's a trivial naming detail. The logic is correct.
🧬 Code Graph Analysis Results
[relevant_code_snippets]
-
File: crates/wallet/crypto/src/memo.rs
Lines: 81-91
Content:pub fn new_pay_ref_and_bytes_truncate<P: AsRef<[u8]>, B: AsRef<[u8]>>(pay_ref: P, msg_bytes: B) -> Option<Self> { let pr = pay_ref.as_ref(); let available_len = (Self::MAX_BYTES_LENGTH - 1).checked_sub(pr.len())?; // -1 for length prefix byte let mb = msg_bytes.as_ref(); let mb = if mb.len() > available_len { &mb[..available_len] } else { mb }; Self::new_pay_ref_and_bytes(pay_ref, mb) } -
File: bindings/src/types/wallet-daemon-client/TransferOutput.ts
Lines: 6-24
Content:export type TransferOutput = { /** * Destination address used to derive the UTXO encryption keys, owner signature and the account in which to * deposit revealed funds */ address: OotleAddress; /** * Amount to spend to a revealed output */ revealed_amount: Amount; /** * Amount to spend to a blinded output */ blinded_amount: Amount; /** * Optional memo to include a memo in the output. This memo is encrypted and can only be read by the recipient. */ memo: Memo | null; }; -
File: bindings/src/types/wallet-daemon-client/Memo.ts
Lines: 3-3
Content:export type Memo = { U256: string } | { Message: string } | { Bytes: string } | { PayRefAndBytes: string };
✏️ Learnings added
Learnt from: sdbondi
Repo: tari-project/tari-ootle PR: 1629
File: applications/tari_walletd/src/handlers/accounts.rs:1001-1002
Timestamp: 2025-11-04T10:10:24.249Z
Learning: In applications/tari_walletd/src/handlers/accounts.rs, the expect() on Memo::new_pay_ref_and_bytes_truncate at line 1002 is safe and intentional. PayRef is validated to be at most 64 bytes during address decoding (PayRef::MAX_LEN = 64), and the function only returns None if payref exceeds 252 bytes (Memo::MAX_BYTES_LENGTH - 1). Since 64 < 252, None is impossible with a valid PayRef.
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.
2 participants
Description
feat!: adds payref to
Ootleaddressfeat: updates JS Ootle Address de/encoder and web UI to generate payref address
Motivation and Context
Allows a payment ref. to be attached to an address. When provided to a compatible wallet, it will include the payment reference in the encrypted output memo for the payment.
How Has This Been Tested?
Manually, existing unit tests
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
UI Improvements