Feat/optional sender - squashed by p2arthur · Pull Request #520 · algorandfoundation/algokit-lora

p2arthur · 2025-11-07T10:20:40Z

feat(optional sender) - squashed

This PR replaces the previous one that contained unsigned commits.
I’ve taken the latest version from that branch, squashed all commits into a single signed commit, and reopened it here to maintain a clean history.

Both this PR and the old one have been moved to draft.
I’ll be pushing new commits here to address all review comments and suggestions made earlier today on the previous PR.

Please continue the discussion and review process on this new PR moving forward. 🚀

cloudflare-workers-and-pages · 2025-11-07T10:26:32Z

Deploying algokit-lora with Cloudflare Pages

Latest commit:	`c5675a2`
Status:	✅ Deploy successful!
Preview URL:	https://7bb95edf.algokit-lora.pages.dev
Branch Preview URL:	https://feat-optional-sender-improve.algokit-lora.pages.dev

View logs

PatrickDinh

Thanks @p2arthur for the work. Above are some comments from me. I created this PR too #523 to address some other minor comments.

PatrickDinh · 2025-11-14T05:33:13Z

src/features/accounts/components/transaction-sender-link.tsx

@@ -0,0 +1,33 @@
+import { Nfd } from '@/features/nfd/data/types'
+


remove empty line

PatrickDinh · 2025-11-14T05:34:41Z

src/features/accounts/components/transaction-sender-link.tsx

+export type Props = PropsWithChildren<{
+  address: string | Address
+  short?: boolean
+  className?: string
+  showCopyButton?: boolean
+  showQRButton?: boolean
+  nfd?: Nfd
+  autoPopulated?: boolean
+}>


this should be type Props = AddressOrNfdLinkProps & { autoPopulated?: boolean }

Much better!

PatrickDinh · 2025-11-14T06:08:03Z

src/features/transaction-wizard/components/asset-create-transaction-builder.tsx

+        manager: asAddressOrNfd(data.manager.value!),
+        reserve: asAddressOrNfd(data.reserve.value!),
+        freeze: asAddressOrNfd(data.freeze.value!),
+        clawback: asAddressOrNfd(data.clawback.value!),


why do you use asAddressOrNfd here? If I recall correctly these values can be optional

You're right, back to optional.

manager: asOptionalAddressOrNfd(data.manager), reserve: asOptionalAddressOrNfd(data.reserve), freeze: asOptionalAddressOrNfd(data.freeze), clawback: asOptionalAddressOrNfd(data.clawback),

PatrickDinh · 2025-11-14T06:08:51Z

src/features/transaction-wizard/components/asset-reconfigure-transaction-builder.tsx

+        manager: asAddressOrNfd(data.manager.value!),
+        reserve: asAddressOrNfd(data.reserve.value!),
+        freeze: asAddressOrNfd(data.freeze.value!),
+        clawback: asAddressOrNfd(data.clawback.value!),


same here, why use asAddressOrNfd?

PatrickDinh · 2025-11-14T06:23:12Z

src/features/transaction-wizard/utils/resolve-sender-address.ts

+    }
+  }
+
+  return {


When the sender can't be resolved, the value is set to '', is it intended? Have you checked the behaviour of the transaction wizard when the value is ''?

Checking for that case like this now

`const val = data?.value ?? ''
const res = data?.resolvedAddress ?? ''

const isEmpty = !val && !res

if (isEmpty) {`

I think we could

return data

if we can't populate the address

PatrickDinh · 2025-11-14T06:42:31Z

src/features/transaction-wizard/transaction-wizard-page.test.tsx

+            )
+
+            const result = await localnet.context.waitForIndexerTransaction(transactionId)
+            expect(result.transaction.sender).toBe(testAccount.addr.toString())


I think you should compare it with the kmd account (from account manager) because I'm not sure is the testAccount is guarantee to be the same.

PatrickDinh · 2025-11-14T06:44:21Z

package-lock.json

-      "version": "4.1.16",
-      "resolved": "https://registry.npmjs.org/@tailwindcss/node/-/node-4.1.16.tgz",
-      "integrity": "sha512-BX5iaSsloNuvKNHRN3k2RcCuTEgASTo77mofW0vmeHkfrDWaoFAFvNHpEgtu0eqyypcyiBkDWzSMxJhp3AUVcw==",
+      "version": "4.1.17",


I think we shouldn't update package-lock in this PR. If it needs to be updated, it should be a separate PR that deals with dependencies

PatrickDinh · 2025-11-14T06:47:37Z

src/features/transaction-wizard/transaction-wizard-page.tsx

        <p>Create and send transactions to the selected network using a connected wallet.</p>
        <TransactionsBuilder
-          defaultTransactions={searchParamsTransactions}
+          key={searchParamsTransactions.transactions.map((t) => t.id).join('|')} // rerender when it gets populated


I don't think this is a good idea, you should wrap this in a loading spinner when it's loading. Have a look at RenderLoadable for inspiration.

Added a PageLoader before rendering the TransactionBuilder. That removes the update based on key change (hacky) approach.

src/features/transaction-wizard/utils/use-transaction-search-params-builder.ts

PatrickDinh · 2025-11-14T06:54:07Z

src/features/transaction-wizard/utils/use-transaction-search-params-builder.ts

-    if (errors && errors.length > 0) {
-      for (const error of errors) {
-        toast.error(error)
+    let cancelled = false


why is cancelled needed here?

Removed together with the unused error handling

PatrickDinh · 2025-11-18T11:19:44Z

src/features/transaction-wizard/utils/resolve-sender-address.ts

+import { betanetId, mainnetId, testnetId, fnetId, localnetId } from '@/features/network/data'
+import { algorandClient } from '@/features/common/data/algo-client'
+
+export default async function resolveSenderAddress(data?: { value?: string; resolvedAddress?: string }): Promise<TransactionSender> {


I didn't pick this up in the previous round, I think we should avoid using export default. We use named export everywhere else in this project.

I think data shouldn't be undefined

PatrickDinh · 2025-11-19T00:10:40Z

src/features/transaction-wizard/utils/resolve-sender-address.ts

+export default async function resolveSenderAddress(data?: { value?: string; resolvedAddress?: string }): Promise<TransactionSender> {
+  const { id: networkId } = settingsStore.get(networkConfigAtom)
+
+  const val = data?.value ?? ''


I think you can simplify this function by

export async function resolveSenderAddress(data: { value?: string; resolvedAddress?: string }): Promise<TransactionSender> { if (data.value) { return data } // The rest }

Makes the logic simple, if the data has value, just return it.

No need data? because data can't be undefined

PatrickDinh · 2025-11-19T00:17:24Z

src/features/transaction-wizard/utils/use-transaction-search-params-builder.ts

-    if (errors && errors.length > 0) {
-      for (const error of errors) {
-        toast.error(error)
+    let mounted = true


maybe I missed something but I don't understand the purpose of the mounted variable. You init it with true and never change the value. Maybe it isn't needed

PatrickDinh · 2025-11-19T00:23:59Z

src/features/transaction-wizard/utils/use-transaction-search-params-builder.ts

-      for (const error of errors) {
-        toast.error(error)
+    let mounted = true
+    const loadTransactions = async () => {


I think you should refactor the async logic inside useEffect. Elsewhere we use jotai atom to deal with loading data. Have a look at the transaction-page.tsx for example. I am aware that it needs to few pieces to setup so if you need a hand, please let me know.

PatrickDinh · 2025-11-19T00:29:40Z

src/features/transaction-wizard/transaction-wizard-page.test.tsx

            const senderInput = await component.findByLabelText(/Sender/)
            fireEvent.input(senderInput, {
-              target: { value: testAccount.addr },
+              target: { value: walletAccount.addr },


Why did you change testAccount to walletAccount here? I thought that walletAccount is only used when the sender isn't set. I don't expect the see any changes in this test to be honest. It shouldn't be affected by the optional sender feature you are working on.

…l sender branch

…e unsigned commits

… main

…params transaction builder

…r in URL params

…esting

…appers

…estWallet + address PR comments

… fields that got required

p2arthur · 2025-11-20T08:32:31Z

src/features/transaction-wizard/utils/resolve-sender-address.ts

+  const val = data?.value ?? ''
+  const res = data?.resolvedAddress ?? ''
+
+  const isEmpty = !val && !res


This is to account for the "" case

…plify the function and make it named export

…o use jotai for async auto population

…o use case wallet variables instead of test suite walletAddress

PatrickDinh · 2025-11-25T01:28:38Z

src/features/transaction-wizard/components/asset-reconfigure-transaction-builder.tsx

        type: BuildableTransactionType.AssetReconfigure,
        asset: data.asset,
-        sender: data.sender,
+


remove this empty line

PatrickDinh · 2025-11-25T01:29:58Z

src/features/transaction-wizard/components/payment-transaction-builder.tsx

 const receiverLabel = 'Receiver'

 export const paymentFormSchema = z.object({
+  sender: optionalAddressFieldSchema,


I think moving the sender above ...commonSchema can caused unwanted bahaviour because this will be overwritten by commonSchema

I agree. Even though there aren't any properties that would overlap, setting the sender: optionalAddress afterwards makes a lot more sense.

PatrickDinh · 2025-11-25T01:55:33Z

src/features/transaction-wizard/mappers/as-algosdk-transactions.ts

  return {
    sender: transaction.sender.resolvedAddress,
-    receiver: transaction.receiver ? transaction.receiver.resolvedAddress : transaction.sender.resolvedAddress,
+    receiver: transaction.receiver ? transaction.receiver.resolvedAddress : (transaction.sender?.resolvedAddress ?? 'LOL IDK'),


I don't think you need to do this, the sender can't be undefined

PatrickDinh · 2025-11-25T02:02:45Z

src/features/transaction-wizard/utils/resolve-sender-address.ts

+import { betanetId, mainnetId, testnetId, fnetId, localnetId } from '@/features/network/data'
+import { algorandClient } from '@/features/common/data/algo-client'
+
+export async function resolveSenderAddress(data: { value?: string; resolvedAddress?: string }): Promise<TransactionSender> {


let's rename to resolveTransactionSender to match the returned type

PatrickDinh · 2025-11-25T02:04:12Z

src/features/transaction-wizard/utils/resolve-sender-address.ts

+export default async function resolveSenderAddress(data?: { value?: string; resolvedAddress?: string }): Promise<TransactionSender> {
+  const { id: networkId } = settingsStore.get(networkConfigAtom)
+
+  const val = data?.value ?? ''


No need data? because data can't be undefined

PatrickDinh · 2025-11-25T03:06:36Z

src/features/transaction-wizard/utils/transactions-url-search-params.test.tsx

    })

    it.each([
      // Missing required field cases


You should remove these comments too if you removed the test case

PatrickDinh · 2025-11-25T03:08:17Z

src/features/transaction-wizard/utils/transactions-url-search-params.test.tsx

+      expect(await screen.findByText(assetId)).toBeInTheDocument()
+      // Find the yellow sender link (auto-populated)
+      const senderLinks = await screen.findAllByText(localnetDispenderAccount.addr.toString())
+      expect(senderLinks.some((link) => link.className.includes('text-yellow-500'))).toBe(true)


Not sure if we need to go this far in the test to be honest, it's just the visual effect. In the future, if we need to change the color, we will need to update all the tests

PatrickDinh · 2025-11-25T03:10:19Z

src/features/transaction-wizard/components/transactions-builder.tsx

+  useEffect(() => {
+    setTransactions(defaultTransactions ?? [])
+  }, [defaultTransactions])


can you confirm if the useEffect is needed. This component isn't rendered until the loadable atom resolves data

Not needed. Removed it

PatrickDinh · 2025-11-25T03:16:19Z

src/features/transactions/data/autopopulate-search-params-transaction.ts

+  return groupedParams.filter((entry) => Object.keys(entry).length > 0)
+}
+
+const autopopulateAddress = atomFamily((searchParamsString: string) => {


can you review the names in this file? They don't match with the purpose.

You don't need an atomFamily here, please refer to the implementation of the useLoadableTransactionAtom. If you have questions, please reach out to me.

PatrickDinh · 2025-11-25T03:19:41Z

.vscode/settings.json

    "editor.defaultFormatter": "esbenp.prettier-vscode"
-  }
+  },
+  "postman.settings.dotenv-detection-notification-visibility": false


why was this added?

chore: refactor the search params transactions hook

… improve optional sender handling

PatrickDinh · 2025-12-17T01:02:33Z

src/features/transaction-wizard/utils/resolve-sender-address.ts

+    return {
+      value: val,
+      resolvedAddress: res,
+    }


nitpick:

return data

if (val) { return data as TransactionSender }

PatrickDinh · 2025-12-17T01:03:19Z

src/features/transaction-wizard/utils/resolve-sender-address.ts

+    }
+  }
+
+  return {


I think we could

return data

if we can't populate the address

PatrickDinh · 2025-12-17T01:03:50Z

src/features/transaction-wizard/utils/transactions-url-search-params.test.tsx

+    })
+
+    it('should render payment transaction without sender - auto populate sender with localnet address', async () => {
+      const localnetDispenderAccount = await localnet.algorand.account.localNetDispenser()


Suggested change

const localnetDispenderAccount = await localnet.algorand.account.localNetDispenser()

const localnetDispenserAccount = await localnet.algorand.account.localNetDispenser()

PatrickDinh · 2025-12-17T01:04:31Z

src/features/transaction-wizard/utils/transactions-url-search-params.test.tsx

+    })
+
+    it('should render asset create transaction without sender - auto populate sender with localnet address', async () => {
+      const localnetDispenderAccount = await localnet.algorand.account.localNetDispenser()


fix spelling for all localnetDispenderAccount

PatrickDinh · 2025-12-17T01:06:24Z

src/features/transaction-wizard/utils/transactions-url-search-params.test.tsx

+      expect(await screen.findByText(`0 ${unitName}`)).toBeInTheDocument()
+    })
+
+    it('should render asset opt-in transaction without sender auto-populated', async () => {


Suggested change

it('should render asset opt-in transaction without sender auto-populated', async () => {

it('should render asset opt-in transaction without sender - auto populate sender with localnet address', async () => {

Let's make it consistent with the rest

PatrickDinh · 2025-12-17T01:07:44Z

src/features/transaction-wizard/utils/transactions-url-search-params.test.tsx

+        }),
+      })
+
+      await waitFor(


why does this test has waitFor but the opt-in test doesn't? Should you use findAllByText here as well to avoid waitFor?

Changed all tests that use waitFor to use await findAllByText instead. Much better!

PatrickDinh · 2025-12-17T01:10:33Z

src/features/transaction-wizard/mappers/as-algosdk-transactions.ts

  return {
    sender: transaction.sender.resolvedAddress,
-    receiver: transaction.receiver ? transaction.receiver.resolvedAddress : transaction.sender.resolvedAddress,
+    receiver: transaction.receiver ? transaction.receiver.resolvedAddress : transaction.sender?.resolvedAddress,


Suggested change

receiver: transaction.receiver ? transaction.receiver.resolvedAddress : transaction.sender?.resolvedAddress,

receiver: transaction.receiver ? transaction.receiver.resolvedAddress : transaction.sender.resolvedAddress,

- throw error when can't resolve sender - clean up the tests

PR feedback

…gorandfoundation/algokit-lora into feat/optional-sender-improved

p2arthur requested a review from PatrickDinh November 11, 2025 06:58

p2arthur marked this pull request as ready for review November 14, 2025 03:49

PatrickDinh requested changes Nov 14, 2025

View reviewed changes

PatrickDinh requested changes Nov 19, 2025

View reviewed changes

p2arthur and others added 17 commits November 19, 2025 12:57

chore(merge-main): merge latest from main with tailwind v4 to optiona…

9f35acb

…l sender branch

feat(optional_sender): Add optional sender at latest squashed to solv…

0e78c0f

…e unsigned commits

chore(package-lock): update package lock to reflect dependencies from…

78166ad

… main

feat(search_params) add optional sender with auto populate to search …

6c2ad60

…params transaction builder

test(optional_sender): add optional sender to url params test

406f776

chore: add DataProvider to txn wizard test

a22d374

chore: lint-fix

bf2ae35

feat(transaction-wizard): auto-populate sender from localnet dispense…

fac30c1

…r in URL params

fix(search_params): fix two tests that were failing only on CI/CD

669d16f

fix(test): add racing condition to wait asset-opt out render before t…

799967c

…esting

fix(test): add racing condition to wait render before testing

02b70e6

fix(test): add racing condition to wait render before testing

a874ce9

reafactor(non-null): remove non-null assertion from the transaction m…

d65e4b4

…appers

test(optional_sender): updated tests to use kmd instead of assuming t…

d7215ae

…estWallet + address PR comments

chore(optional_sender): cleanup to remove repeated tests and optional…

cfe5917

… fields that got required

fix(conflicts) fix merge conflicts from feature branch

a7a8d85

fix(conflicts) fix merge conflicts from feature branch

cc92c31

p2arthur force-pushed the feat/optional-sender-improved branch from 235900a to cc92c31 Compare November 19, 2025 21:05

p2arthur commented Nov 20, 2025

View reviewed changes

p2arthur added 3 commits November 20, 2025 01:09

refactor(resolve_address): refactor the resolve sender address to sim…

bb29210

…plify the function and make it named export

refactor(use-search-params): refactor use search params transaction t…

fb2a694

…o use jotai for async auto population

refactor(use-search-params): refactor transactions wizard page test t…

444a3cf

…o use case wallet variables instead of test suite walletAddress

PatrickDinh requested changes Nov 25, 2025

View reviewed changes

chore: refactor the search params transactions hook

2c44d4d

p2arthur and others added 5 commits November 27, 2025 22:13

Merge pull request #531 from algorandfoundation/jotai-refactor

907ed2c

chore: refactor the search params transactions hook

refactor: rename resolveSenderAddress to resolveTransactionSender and…

aa66da7

… improve optional sender handling

fix: fix failing tests for auto populating and simulating transactions

4f57874

fix: lint error blocking CI

55986c0

test: add wait function to wait for the sender to get populated

8355622

PatrickDinh reviewed Dec 17, 2025

View reviewed changes

p2arthur added 2 commits December 17, 2025 00:47

fix: fix dispenser spelling+ small refactors

748168e

fix: fix npm audit error by extending acceptance of vulnerability

c81b576

p2arthur requested a review from PatrickDinh December 17, 2025 09:18

PatrickDinh and others added 4 commits December 18, 2025 09:19

chore: pr feedback

467771c

- throw error when can't resolve sender - clean up the tests

Merge pull request #523 from algorandfoundation/pr-feedback

4c6653a

PR feedback

Merge branch 'main' into feat/optional-sender-improved

68c4dce

Merge branch 'feat/optional-sender-improved' of https://github.com/al…

c5675a2

…gorandfoundation/algokit-lora into feat/optional-sender-improved

PatrickDinh approved these changes Dec 18, 2025

View reviewed changes

p2arthur merged commit 5b4b51f into main Dec 18, 2025
2 checks passed

		@@ -0,0 +1,33 @@
		import { Nfd } from '@/features/nfd/data/types'

	const localnetDispenderAccount = await localnet.algorand.account.localNetDispenser()
	const localnetDispenserAccount = await localnet.algorand.account.localNetDispenser()

	it('should render asset opt-in transaction without sender auto-populated', async () => {
	it('should render asset opt-in transaction without sender - auto populate sender with localnet address', async () => {

	receiver: transaction.receiver ? transaction.receiver.resolvedAddress : transaction.sender?.resolvedAddress,
	receiver: transaction.receiver ? transaction.receiver.resolvedAddress : transaction.sender.resolvedAddress,

Conversation

p2arthur commented Nov 7, 2025

Uh oh!

cloudflare-workers-and-pages bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying algokit-lora with Cloudflare Pages

Uh oh!

PatrickDinh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Nov 7, 2025 •

edited

Loading