-
Notifications
You must be signed in to change notification settings - Fork 19
Feat/optional sender #491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feat/optional sender #491
Conversation
…lora into feat/optional-sender
…nder address with auto populate using feeSink addr
… sender in localnet
…action types - 7 missing
Deploying algokit-lora with
|
Latest commit: |
206971b
|
Status: | ✅ Deploy successful! |
Preview URL: | https://2890fb69.algokit-lora.pages.dev |
Branch Preview URL: | https://feat-optional-sender.algokit-lora.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments. I'll resume my review later.
{autoPopulated && ( | ||
<span className="group ml-1 cursor-help text-yellow-500"> | ||
<span>?</span> | ||
<div className="absolute z-10 hidden rounded-sm border-2 border-gray-300/20 p-1 group-hover:block">auto populated</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL group-hover
<div className="flex items-center overflow-hidden"> | ||
{link} | ||
{autoPopulated && ( | ||
<span className="group ml-1 cursor-help text-yellow-500"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: there is double spaces here
<dl key={index} className={cn('grid grid-cols-subgrid col-span-2')}> | ||
<dt className={cn('font-medium', dtClassName)}>{item.dt}</dt> | ||
<dd className={cn('overflow-ellipsis whitespace-normal overflow-hidden')}> | ||
<dd className={cn('overflow-ellipsis whitespace-normal overflow-hidden', item.highlightedDD && 'font-medium text-green-500')}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be implemented as a ddClassNames
props, i.e.
<dd className={cn('overflow-ellipsis whitespace-normal overflow-hidden', item.ddClassNames)}
the consumer if this component can decide how they want to style it. This way it's more flexible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, will try that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest we don't need that change. I think it slipped while I was trying to figure out the inner workings of the transaction wizard in order to add highlighting if auto populated.
Reverting this change
export const SenderSchema = z.object({ | ||
value: z.string().optional(), | ||
resolvedAddress: z.string().optional(), | ||
}) | ||
|
||
// TODO Arthur - Added this shape to make the sender optional in the forms that required it | ||
export const optionalSenderFieldShape = { | ||
sender: SenderSchema, | ||
} as const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at this code for a while so maybe I missed something.
Something isn't quite right here because the value
field will miss the isAddress
and isNfd
validations.
Is it possible to use the optionalAddressFieldSchema
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id: transaction?.id ?? randomGuid(), | ||
type: BuildableTransactionType.AccountClose, | ||
sender: data.sender, | ||
sender: await defineSenderAddress(data.sender!, networkId), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need ! in data.sender!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the file name should be snake-case to follow the project file name convention
|
||
const ensureSender = (sender: AddressOrNfd | undefined) => { | ||
const validSender = sender ?? { | ||
value: 'TGIPEOKUFC5JFTPFMXGSZWOGOFA7THFZXUTRLQEOH3RD3LGI6QEEWJNML4', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this address? you should avoid using magic string and define a constant?
export type AddressOrNfd = { | ||
value: Address | Nfd | ||
resolvedAddress: Address | ||
autoPopulated?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding autoPopulated
field to this type doesn't feel right. This type is used across the app to represent an address or nfd, it shouldn't have to concern about whether the data was auto populated or not.
As a suggestion, I'd create another type
export type TransactionSender {
value: AddressOrNfd
autoPopulated: boolean
}
to represent this logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type CommonBuildTransactionResult = { | ||
id: string | ||
sender: AddressOrNfd | ||
sender: AddressOrNfd | undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be undefined
? maybe I missed something but after a quick look I see that the value of the sender
field is always populated with defineSenderAddress
function
dt: 'Sender', | ||
dd: <AddressOrNfdLink address={params.sender} />, | ||
dd: ( | ||
<AddressOrNfdLink |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar logic to the comment on AddressOrNfd
type, I think we should create another component that wraps the AddressOrNfdLink
component to display the sender of a transaction.
resolvedAddress: 'TGIPEOKUFC5JFTPFMXGSZWOGOFA7THFZXUTRLQEOH3RD3LGI6QEEWJNML4', | ||
} | ||
|
||
invariant(validSender, 'Sender must be set') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this invariant
is not needed, validSender
must be a truthy from above.
There is an UX issue that I think we should review, in the below recording:
|
transactions.forEach((transaction) => { | ||
if (transaction.sender?.autoPopulated === true) setOnlySimulateOptionalSender(true) | ||
}) | ||
|
||
if (onlySimulateOptionalSender) { | ||
return { | ||
disabled: true, | ||
disabledReason: onlySimulateOptionalSenderMessage, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that onlySimulateOptionalSender
is only used within this useMemo
, I don't think you need a separate onlySimulateOptionalSender
state
{table.getRowModel().rows?.length ? ( | ||
table | ||
.getRowModel() | ||
.rows.map((row) => ( | ||
table.getRowModel().rows.map((row) => { | ||
return ( | ||
<TransactionRow | ||
key={row.id} | ||
row={row} | ||
transactionPositions={transactionPositions} | ||
onEditTransaction={onEditTransaction} | ||
onEditResources={onEditResources} | ||
/> | ||
)) | ||
) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be changed? If it doesn't need to be, I suggest to revert it to make PR review easier.
|
||
import defineSenderAddress from '../utils/define-sender-address' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need an extra empty line above this. Also, I think resolveSenderAddress
is a better name. The logic inside this function doesn't define anything new.
import { TransactionBuilderNoteField } from './transaction-builder-note-field' | ||
import { asAddressOrNfd } from '../mappers/as-address-or-nfd' | ||
import { ActiveWalletAccount } from '@/features/wallet/types/active-wallet' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove empty line here
field: 'sender', | ||
label: 'Sender', | ||
helpText: 'Account to pay from. Sends the transaction and pays the fee', | ||
helpText: 'Account to pay from. Sends the transaction and pays the fee - optional for simulating', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why the helpText is updated here but not for other transaction types?
…e lora transaction wizard
… transaction wizard page
) : ( | ||
<div className="flex items-center overflow-hidden"> | ||
{link} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: no need this newline
export default function TransactionSenderLink({ | ||
address, | ||
short, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this empty line
import { Address } from '../data/types' | ||
import { cn } from '@/features/common/utils' | ||
|
||
export type AddressOrNfdLinkProps = PropsWithChildren<{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this type to avoid conflicts with address-or-ndf-link.tsx
. If it isn't used anywhere else, I'd just call it Props
export type AddressOrNfdLinkProps = PropsWithChildren<{ | |
type Props = PropsWithChildren<{ |
pera: 'Pera', | ||
exodus: 'Exodus', | ||
lute: 'Lute', | ||
// The below providers aren't used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should still keep this comment
resolvedAddress: addressOrAccount, | ||
} satisfies AddressOrNfd | ||
} | ||
export const asOptionalAddressOrNfd = (transactionSender?: Partial<TransactionSender>): TransactionSender => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function doesn't look right, the name is asOptionalAddressOrNfd
but return TransactionSender
:
- The type in the function name doesn't match with the return type
- The name implies optional but the return type is not
I think you should create a new function just to handle transaction sender and revert this back to the original state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a quick hack. I think you can use something like below
export const asTransactionSender = (transactionSender?: Partial<TransactionSender>): TransactionSender | undefined => {
if (!transactionSender) return undefined
if (transactionSender.autoPopulated) return undefined
return transactionSender.value && transactionSender.resolvedAddress
? ({
value: transactionSender.value,
resolvedAddress: transactionSender.resolvedAddress,
autoPopulated: false,
} satisfies TransactionSender)
: undefined
}
You will need to test to ensure it works. Additionally, it should be in a new file as-transaction-sender
sender: await resolveSenderAddress(data.sender), | ||
closeRemainderTo: data.closeRemainderTo, | ||
receiver: asOptionalAddressOrNfd(data.receiver), | ||
receiver: { value: data.receiver.value!, resolvedAddress: data.receiver.resolvedAddress! }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why the logic for receiver needed to be updated, maybe it was related to the asOptionalAddressOrNfd
function I commented on earlier, you should revert this.
export type BuildPaymentTransactionResult = CommonBuildTransactionResult & { | ||
type: BuildableTransactionType.Payment | ||
receiver: AddressOrNfd | ||
sender: AddressOrNfd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should remove this, the sender
is already in CommonBuildTransactionResult
return { | ||
sender: transaction.sender.resolvedAddress, | ||
receiver: transaction.receiver ? transaction.receiver.resolvedAddress : transaction.sender.resolvedAddress, | ||
sender: ensureSender(transaction.sender), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to call ensureSender
anymore?
const sendButton = await waitFor(() => { | ||
const sendButton = component.getByRole('button', { name: sendButtonLabel }) | ||
expect(sendButton).not.toBeDisabled() | ||
return sendButton! | ||
}) | ||
await user.click(sendButton) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that if the sender is auto populated only "Simulate" is allowed but this test seems to be able to "Send" transaction, can you please confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you had a chance to look into this?
src/features/transaction-wizard/transaction-wizard-page.test.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Hoang Dinh <[email protected]>
export type Props = PropsWithChildren<{ | ||
address: Address | ||
short?: boolean | ||
className?: string | ||
showCopyButton?: boolean | ||
showQRButton?: boolean | ||
nfd?: Nfd | ||
autoPopulated?: boolean | ||
}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick
export type Props = PropsWithChildren<{ | |
address: Address | |
short?: boolean | |
className?: string | |
showCopyButton?: boolean | |
showQRButton?: boolean | |
nfd?: Nfd | |
autoPopulated?: boolean | |
}> | |
export type Props = AddressOrNfdLinkProps & { | |
autoPopulated?: boolean | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type of the address field is incorrect, it should be
address: string | Address;
The Address
is from algosdk
<AddressOrNfdLink | ||
address={address} | ||
short={short} | ||
className={cn(autoPopulated && 'text-yellow-500')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the className
prop needs to be passed in here too, I think this would become something like
className={cn(className, autoPopulated && 'text-yellow-500')}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can rewrite this bit to
export default function TransactionSenderLink(props: Props) {
const { autoPopulated, className, ...rest } = props
return (
<div className="flex items-center">
<AddressOrNfdLink className={cn(className, autoPopulated && 'text-yellow-500')} {...rest} />
...
sender: await resolveSenderAddress(data.sender), | ||
closeRemainderTo: data.closeRemainderTo, | ||
receiver: asOptionalAddressOrNfd(data.receiver), | ||
receiver: asAddressOrNfd(data.receiver.value!), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something isn't right here. This change potentially breaks existing logic. On main
, the receiver field is
receiver: asOptionalAddressOrNfd(data.receiver),
this is an optional field.
In this PR, it was changed to
receiver: asAddressOrNfd(data.receiver.value!),
Some concerns:
- Now it isn't an optional field anymore. This is incorrect. For an account close transaction, the receiver field is: "Account to receive the amount. Leave blank if 'Close remainder to' account should receive the full balance"
|
||
export const senderFieldSchema = { sender: addressFieldSchema } | ||
|
||
// TODO Arthur - Added this shape to make the sender optional in the forms that required it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this TODO anymore?
approvalProgram: transaction.approvalProgram, | ||
clearStateProgram: transaction.clearStateProgram, | ||
sender: transaction.sender, | ||
sender: asTransactionSender(transaction.sender), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need the method asTransactionSender
, I could be completely wrong here but you should try something simple like
sender: transaction.sender?.autoPopulated ? undefined : transaction.sender
In the above logic, the sender should be set to undefined if it was autoPopulated
import { betanetId, mainnetId, testnetId, fnetId, localnetId } from '@/features/network/data' | ||
import { algorandClient } from '@/features/common/data/algo-client' | ||
|
||
export default async function resolveSenderAddress<T extends OptionalSenderFieldSchema>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the generic <T extends ...>
required here?
|
||
export default async function resolveSenderAddress<T extends OptionalSenderFieldSchema>( | ||
data: T | ||
): Promise<AddressOrNfd & TransactionSender> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can just be Promise<TransactionSender>
}, [activeAddress, commonButtonDisableProps, requireSignaturesOnSimulate]) | ||
|
||
const sendButtonDisabledProps = useMemo(() => { | ||
// derive it, don't store it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this comment here?
export const assetOptOutFormSchema = z.object({ | ||
...commonSchema, | ||
...senderFieldSchema, | ||
...optionalSenderFieldShape, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can skip the optionalSenderFieldShape
and use
sender: optionalAddressFieldSchema
directly. It's just 1 line of code and it will make reading a lot easier
export const senderFieldSchema = { sender: addressFieldSchema } | ||
|
||
// TODO Arthur - Added this shape to make the sender optional in the forms that required it | ||
export const optionalSenderFieldShape = { | ||
sender: optionalAddressFieldSchema, | ||
} as const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the new const optionalSenderFieldShape
introduced? Since this PR changing sender to optional, I think you can update
export const senderFieldSchema = { sender: optionalAddressFieldSchema }
export type Props = PropsWithChildren<{ | ||
address: Address | ||
short?: boolean | ||
className?: string | ||
showCopyButton?: boolean | ||
showQRButton?: boolean | ||
nfd?: Nfd | ||
autoPopulated?: boolean | ||
}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type of the address field is incorrect, it should be
address: string | Address;
The Address
is from algosdk
{ | ||
dt: 'Sender', | ||
dd: <AddressOrNfdLink address={params.sender} />, | ||
dd: <TransactionSenderLink autoPopulated={txn.sender.autoPopulated} address={String(params.sender)} />, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After you fixed the TransactionSenderLink
, you can replace String(params.sender)
with params.sender
value: Address | Nfd | undefined | ||
resolvedAddress: Address | undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these fields be undefined
? I think not
|
||
return { | ||
sender: transaction.sender.resolvedAddress, | ||
sender: transaction.sender.resolvedAddress!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the !
is needed here, feel like it is a result of the incorrect type
export type TransactionSender = {
value: Address | Nfd | undefined
resolvedAddress: Address | undefined
autoPopulated?: boolean
}
that I pointed out earlier
value: params.sender, | ||
resolvedAddress: params.sender, | ||
}, | ||
sender: asTransactionSender({ value: params.sender!, resolvedAddress: params.sender!, autoPopulated: false }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolveSenderAddress
needs to be call here. This user flow should work exactly like when a user creates the transaction from the transaction wizard
#Optional Sender for Simulation
Status: Open
Goal: Let users simulate transactions without manually entering a sender. If the sender is left empty, we auto-resolve the “best” account by network. The Send button still requires explicit senders on all transactions.
Summary of Changes
bg-red-50
) and show an indicator/tooltip:“This address was auto-populated for simulation.”
Integrated Builders (10/15)
Acceptance Criteria (Quick Map)
Testing Notes
Follow-ups (Post-Draft)
defineSenderAddress
per network.