Skip to content

Conversation

yhabib
Copy link
Contributor

@yhabib yhabib commented Sep 17, 2025

Motivation

The memo field in an ICP transaction modal enables specific advanced user flows. This PR makes this field visible within the transaction modal after it is activated through a Command Palette command.

It supports both types of accounts: ICP and ICRC1

Screen.Recording.2025-10-04.at.22.03.35.mov

Some examples of transactions using the defined memo field can be found here

Changes

  • Adds new option to the Command Palette to show/hide the memo field.
  • Adds a new input for the memo field in the ICPTransactionModal if the option was previously turned on.

Tests

  • Unit tests to check the presence of the field based on the option.

Todos

  • Accessibility (a11y) – any impact?
  • Changelog – is it needed?

@yhabib yhabib force-pushed the yhabib/transactions/memo branch from 25af712 to 206ed35 Compare September 17, 2025 15:41
@yhabib yhabib requested a review from Copilot September 18, 2025 09:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a toggleable memo field option for ICP transactions through the Command Palette. Users can show or hide a memo input field in transaction modals, enabling advanced transaction workflows while keeping the interface clean by default.

  • Adds Command Palette options to show/hide the transaction memo field
  • Implements memo input field in ICP transaction modal when enabled
  • Updates transaction utilities to handle memo text display

Reviewed Changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
frontend/src/lib/stores/transaction-memo-option.store.ts Creates new store for managing memo field visibility
frontend/src/lib/utils/alfred.utils.ts Adds Command Palette actions for toggling memo field
frontend/src/lib/modals/accounts/IcpTransactionModal.svelte Implements conditional memo input and review display
frontend/src/lib/services/icp-accounts.services.ts Updates transfer service to handle memo parameter
frontend/src/lib/utils/icp-transactions.utils.ts Adds memo text mapping for ICP transactions
frontend/src/lib/utils/icrc-transactions.utils.ts Adds memo text mapping for ICRC transactions
frontend/src/lib/components/accounts/TransactionCard.svelte Displays memo text in transaction cards when enabled
frontend/src/lib/i18n/en.json Adds internationalization strings for memo functionality

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@yhabib yhabib force-pushed the yhabib/transactions/memo branch from 06a9184 to 7ce3cc7 Compare October 4, 2025 11:21
@yhabib yhabib force-pushed the yhabib/transactions/memo branch from 5d90cb6 to 64f7cd6 Compare October 4, 2025 18:51
@yhabib yhabib requested a review from Copilot October 4, 2025 18:52
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@yhabib yhabib force-pushed the yhabib/transactions/memo branch 2 times, most recently from f69c4e2 to dd86a80 Compare October 4, 2025 19:17
@yhabib yhabib requested a review from Copilot October 4, 2025 19:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@yhabib yhabib force-pushed the yhabib/transactions/memo branch from dd86a80 to 080bdf6 Compare October 4, 2025 20:03
@yhabib yhabib marked this pull request as ready for review October 4, 2025 20:18
@yhabib yhabib requested a review from a team as a code owner October 4, 2025 20:18
@yhabib yhabib requested a review from Copilot October 6, 2025 05:09
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 85 to 94
<Input
testId="transaction-memo-input"
name="memo"
inputType="number"
step={1}
placeholderLabelKey={$i18n.accounts.icp_transaction_memo_label}
bind:value={memo}
autocomplete="off"
showInfo
>
Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

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

The memo field is configured as a number input with step={1}, but memo values can be arbitrary strings or hex values. Consider using inputType='text' to allow more flexible memo formats.

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@mstrasinskis mstrasinskis left a comment

Choose a reason for hiding this comment

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

Thanks, looks good, just a couple of non-blocking comments.


const memoText = nonNullish(icrc1Memo)
? uint8ArrayToHexString(icrc1Memo)
: !isNullish(memo)
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: nonNullish?

<Identifier size="medium" {label} identifier={otherParty} />
{/if}
{#if nonNullish(memoText) && $transactionMemoOptionStore === "show"}
<p class="value memo" data-tid="transaction-memo"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a plan to use this id data-tid="transaction-memo"?

}

const feeE8s = get(mainTransactionFeeE8sStore);
const icpMemo = nonNullish(memo) ? BigInt(memo) : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick double-check — is it safe to use BigInt(memo) without wrapping it in a try/catch?

github-merge-queue bot pushed a commit that referenced this pull request Oct 8, 2025
# Motivation

Support the transaction memo for both ICP and ICRC1 addresses for ICP
transaction flows. This PR adds utils to validate the provided memo:
* The `memo` field for ICP account transactions is an unsigned 64-bit
integer.
* The `icrc1Memo` field for ICRC1 account transactions is a byte array
with a length of up to 32 bytes.

# Changes

- Updated `mapIcpTransactionToUi` to prepare the `memo` field for the UI
when the value is present. It check first for `icrc1Memo` as this value
is optional, if not present it uses the value from the `memo` field that
is always present.
- Added `isValidIcpMemo`, `isValidIcrc1Memo`, and
`validateTransactionMemo` to validate the different types of memos.

# Tests

- Added unit tests for the utils
- Tested the completed feature in #7351.

# Todos

- [x] Accessibility (a11y) – any impact?
- [x] Changelog – is it needed?
@yhabib yhabib marked this pull request as draft October 8, 2025 10:39
@yhabib
Copy link
Contributor Author

yhabib commented Oct 10, 2025

Duplicated

@yhabib yhabib closed this Oct 10, 2025
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.

2 participants