Skip to content
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.

Sign up for GitHub

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

fix: Disable Add to Batch button for safe apps [SW-363] #4623

Merged
merged 3 commits into from
Dec 10, 2024
Merged

fix: Disable Add to Batch button for safe apps [SW-363] #4623

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa1e69a
fix: Hide Add to Batch button when creating transactions through safe…
usame-algan Dec 6, 2024
6d50adf
fix: Add more SignForm tests
usame-algan Dec 6, 2024
fe87c47
fix: Disable batch button for safe apps transactions
usame-algan Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/components/tx/SignOrExecuteForm/SignForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ export const SignForm = ({
const submitDisabled =
!safeTx || !isSubmittable || disableSubmit || cannotPropose || (needsRiskConfirmation && !isRiskConfirmed)

const isSafeAppTransaction = !!origin
Copy link
Member

Choose a reason for hiding this comment

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

I think origin is also non-null in case of WalletConnect txs and probably swaps/staking/bridging as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should hide it for those transactions as well to be on the safe side. At least for swaps the button is already disabled since it is a delegate call:
Screenshot 2024-12-09 at 10 29 49


return (
<form onSubmit={handleSubmit}>
{hasSigned && <ErrorMessage level="warning">You have already signed this transaction.</ErrorMessage>}
Expand Down Expand Up @@ -130,7 +132,7 @@ export const SignForm = ({
spacing={{ xs: 2, md: 2 }}
>
{/* Batch button */}
{isCreation && !isBatch && (
{isCreation && !isBatch && !isSafeAppTransaction && (
<BatchButton
onClick={onBatchClick}
Copy link

Choose a reason for hiding this comment

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

The condition for rendering BatchButton has been modified, potentially affecting its visibility logic. Review if excluding !isSafeAppTransaction in the surrounding condition and adding it to the disabled prop aligns with the desired behavior.

disabled={submitDisabled || !isBatchable}
Copy link

Choose a reason for hiding this comment

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

Ensure the new isSafeAppTransaction variable is meaningful. Its usage is currently limited to a check condition in the render block. Consider consolidating this logic and removing it if it doesn't add clarity or functionality.

Expand Down
Loading