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.

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

[Counterfactual] Safe creation #3180

Merged
merged 12 commits into from
Feb 8, 2024
Merged
50 changes: 50 additions & 0 deletions src/components/new-safe/create/logic/utils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import * as creationUtils from '@/components/new-safe/create/logic/index'
import { getAvailableSaltNonce } from '@/components/new-safe/create/logic/utils'
import * as web3Utils from '@/hooks/wallets/web3'
import { faker } from '@faker-js/faker'
import type { DeploySafeProps } from '@safe-global/protocol-kit'
import { BrowserProvider, type Eip1193Provider } from 'ethers'

describe('getAvailableSaltNonce', () => {
jest.spyOn(creationUtils, 'computeNewSafeAddress').mockReturnValue(Promise.resolve(faker.finance.ethereumAddress()))

let mockProvider: BrowserProvider
let mockDeployProps: DeploySafeProps

beforeAll(() => {
mockProvider = new BrowserProvider(jest.fn() as unknown as Eip1193Provider)
mockDeployProps = {
safeAccountConfig: {
threshold: 1,
owners: [faker.finance.ethereumAddress()],
fallbackHandler: faker.finance.ethereumAddress(),
},
}
})

beforeEach(() => {
jest.clearAllMocks()
})

it('should return initial nonce if no contract is deployed to the computed address', async () => {
jest.spyOn(web3Utils, 'isSmartContract').mockReturnValue(Promise.resolve(false))
const initialNonce = faker.string.numeric()

const result = await getAvailableSaltNonce(mockProvider, { ...mockDeployProps, saltNonce: initialNonce })

expect(result).toEqual(initialNonce)
})

it('should return an increased nonce if a contract is deployed to the computed address', async () => {
jest.spyOn(web3Utils, 'isSmartContract').mockReturnValueOnce(Promise.resolve(true))
const initialNonce = faker.string.numeric()

const result = await getAvailableSaltNonce(mockProvider, { ...mockDeployProps, saltNonce: initialNonce })

jest.spyOn(web3Utils, 'isSmartContract').mockReturnValueOnce(Promise.resolve(false))

const increasedNonce = (Number(initialNonce) + 1).toString()

expect(result).toEqual(increasedNonce)
})
})
17 changes: 17 additions & 0 deletions src/components/new-safe/create/logic/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { computeNewSafeAddress } from '@/components/new-safe/create/logic/index'
import { isSmartContract } from '@/hooks/wallets/web3'
import type { DeploySafeProps } from '@safe-global/protocol-kit'
import type { BrowserProvider } from 'ethers'

export const getAvailableSaltNonce = async (provider: BrowserProvider, props: DeploySafeProps): Promise<string> => {
const safeAddress = await computeNewSafeAddress(provider, props)
const isContractDeployed = await isSmartContract(provider, safeAddress)
Copy link
Member

Choose a reason for hiding this comment

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

Could we not run into Rate Limit issues here? If someone deployed e.g. 10-20 Safes already?

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 haven't had any issues so far with around 10 safes created but we should handle possible error responses from the RPC call. In case this becomes an issue with users we could still optimize this e.g. by not incrementally checking saltNonces but using something like binary search.

Copy link
Member Author

@usame-algan usame-algan Feb 6, 2024

Choose a reason for hiding this comment

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

If the RPC call throws we can't compute a safeAddress and thus can't create a safe so I would display an error to the user.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm but in that case the user would not be able to create new Safes for that owner right?
If every Safe creation attempts hits the Rate limit, the user will see the error on every attempt. We should experiment if this ever happens by i.e. creating 30 Safes for an owner.

Copy link
Member Author

@usame-algan usame-algan Feb 7, 2024

Choose a reason for hiding this comment

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

I don't think this will realistically happen. What is the use-case for a wallet to create the exact same safe setup 30 times? I agree though that we should try to limit test it cc. @francovenica

Copy link
Member Author

Choose a reason for hiding this comment

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

We could also add a fallback to the readonly provider in case the wallet provider fails to increase the chances of success.


// Safe is already deployed so we try the next saltNonce
if (isContractDeployed) {
return getAvailableSaltNonce(provider, { ...props, saltNonce: (Number(props.saltNonce) + 1).toString() })
}

// We know that there will be a saltNonce but the type has it as optional
return props.saltNonce!
}
148 changes: 89 additions & 59 deletions src/components/new-safe/create/steps/ReviewStep/index.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
import { getAvailableSaltNonce } from '@/components/new-safe/create/logic/utils'
import ErrorMessage from '@/components/tx/ErrorMessage'
import { createCounterfactualSafe } from '@/features/counterfactual/utils'
import useWalletCanPay from '@/hooks/useWalletCanPay'
import { useAppDispatch } from '@/store'
import { FEATURES } from '@/utils/chains'
import { useRouter } from 'next/router'
import { useMemo, useState } from 'react'
import { Button, Grid, Typography, Divider, Box, Alert } from '@mui/material'
import lightPalette from '@/components/theme/lightPalette'
import ChainIndicator from '@/components/common/ChainIndicator'
import EthHashInfo from '@/components/common/EthHashInfo'
import { useCurrentChain } from '@/hooks/useChains'
import { useCurrentChain, useHasFeature } from '@/hooks/useChains'
import useGasPrice, { getTotalFee } from '@/hooks/useGasPrice'
import { useEstimateSafeCreationGas } from '@/components/new-safe/create/useEstimateSafeCreationGas'
import { formatVisualAmount } from '@/utils/formatters'
Expand Down Expand Up @@ -101,10 +106,13 @@ const ReviewStep = ({ data, onSubmit, onBack, setStep }: StepRenderProps<NewSafe
const chain = useCurrentChain()
const wallet = useWallet()
const provider = useWeb3()
const dispatch = useAppDispatch()
const router = useRouter()
const [gasPrice] = useGasPrice()
const saltNonce = useMemo(() => Date.now(), [])
const [_, setPendingSafe] = usePendingSafe()
const [executionMethod, setExecutionMethod] = useState(ExecutionMethod.RELAY)
const [submitError, setSubmitError] = useState<string>()
const isCounterfactualEnabled = useHasFeature(FEATURES.COUNTERFACTUAL)

const ownerAddresses = useMemo(() => data.owners.map((owner) => owner.address), [data.owners])
const [minRelays] = useLeastRemainingRelays(ownerAddresses)
Expand All @@ -117,9 +125,9 @@ const ReviewStep = ({ data, onSubmit, onBack, setStep }: StepRenderProps<NewSafe
return {
owners: data.owners.map((owner) => owner.address),
threshold: data.threshold,
saltNonce,
saltNonce: Date.now(), // This is not the final saltNonce but easier to use and will only result in a slightly higher gas estimation
}
}, [data.owners, data.threshold, saltNonce])
}, [data.owners, data.threshold])

const { gasLimit } = useEstimateSafeCreationGas(safeParams)

Expand All @@ -133,35 +141,50 @@ const ReviewStep = ({ data, onSubmit, onBack, setStep }: StepRenderProps<NewSafe
? formatVisualAmount(getTotalFee(maxFeePerGas, maxPriorityFeePerGas, gasLimit), chain?.nativeCurrency.decimals)
: '> 0.001'

// Only 1 out of 1 safe setups are supported for now
const isCounterfactual = data.threshold === 1 && data.owners.length === 1 && isCounterfactualEnabled
Copy link
Member

Choose a reason for hiding this comment

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

Please try to extract all the logic into a service and only do UI rendering in components.


const handleBack = () => {
onBack(data)
}

const createSafe = async () => {
if (!wallet || !provider || !chain) return

const readOnlyFallbackHandlerContract = await getReadOnlyFallbackHandlerContract(chain.chainId, LATEST_SAFE_VERSION)
try {
const readOnlyFallbackHandlerContract = await getReadOnlyFallbackHandlerContract(
chain.chainId,
LATEST_SAFE_VERSION,
)

const props: DeploySafeProps = {
safeAccountConfig: {
threshold: data.threshold,
owners: data.owners.map((owner) => owner.address),
fallbackHandler: await readOnlyFallbackHandlerContract.getAddress(),
},
saltNonce: saltNonce.toString(),
}
const props: DeploySafeProps = {
safeAccountConfig: {
threshold: data.threshold,
owners: data.owners.map((owner) => owner.address),
fallbackHandler: await readOnlyFallbackHandlerContract.getAddress(),
},
}

const safeAddress = await computeNewSafeAddress(provider, props)
const saltNonce = await getAvailableSaltNonce(provider, { ...props, saltNonce: '0' })
const safeAddress = await computeNewSafeAddress(provider, { ...props, saltNonce })

const pendingSafe = {
...data,
saltNonce,
safeAddress,
willRelay,
}
if (isCounterfactual) {
createCounterfactualSafe(chain, safeAddress, saltNonce, data, dispatch, props, router)
return
}

const pendingSafe = {
...data,
saltNonce: Number(saltNonce),
safeAddress,
willRelay,
}

setPendingSafe(pendingSafe)
onSubmit(pendingSafe)
setPendingSafe(pendingSafe)
onSubmit(pendingSafe)
} catch (_err) {
setSubmitError('Error creating the Safe Account. Please try again later.')
}
}

const isSocialLogin = isSocialLoginWallet(wallet?.label)
Expand Down Expand Up @@ -203,50 +226,57 @@ const ReviewStep = ({ data, onSubmit, onBack, setStep }: StepRenderProps<NewSafe
</Grid>
</Box>

<Divider />
<Box className={layoutCss.row} display="flex" flexDirection="column" gap={3}>
{canRelay && !isSocialLogin && (
<Grid container spacing={3}>
<ReviewRow
name="Execution method"
value={
<ExecutionMethodSelector
executionMethod={executionMethod}
setExecutionMethod={setExecutionMethod}
relays={minRelays}
{!isCounterfactual && (
<>
<Divider />
<Box className={layoutCss.row} display="flex" flexDirection="column" gap={3}>
{canRelay && !isSocialLogin && (
<Grid container spacing={3}>
<ReviewRow
name="Execution method"
value={
<ExecutionMethodSelector
executionMethod={executionMethod}
setExecutionMethod={setExecutionMethod}
relays={minRelays}
/>
}
/>
}
/>
</Grid>
)}
</Grid>
)}

<Grid data-testid="network-fee-section" container spacing={3}>
<ReviewRow
name="Est. network fee"
value={
<>
<NetworkFee totalFee={totalFee} willRelay={willRelay} chain={chain} />

{!willRelay && !isSocialLogin && (
<Typography variant="body2" color="text.secondary" mt={1}>
You will have to confirm a transaction with your connected wallet.
</Typography>
)}
</>
}
/>
</Grid>
<Grid data-testid="network-fee-section" container spacing={3}>
<ReviewRow
name="Est. network fee"
schmanu marked this conversation as resolved.
Show resolved Hide resolved
value={
<>
<NetworkFee totalFee={totalFee} willRelay={willRelay} chain={chain} />

{isWrongChain && <NetworkWarning />}
{!willRelay && !isSocialLogin && (
<Typography variant="body2" color="text.secondary" mt={1}>
You will have to confirm a transaction with your connected wallet.
</Typography>
)}
</>
}
/>
</Grid>

{!walletCanPay && !willRelay && (
<ErrorMessage>Your connected wallet doesn&apos;t have enough funds to execute this transaction</ErrorMessage>
)}
</Box>
{isWrongChain && <NetworkWarning />}

{!walletCanPay && !willRelay && (
<ErrorMessage>
Your connected wallet doesn&apos;t have enough funds to execute this transaction
</ErrorMessage>
)}
</Box>
</>
)}

<Divider />

<Box className={layoutCss.row}>
{submitError && <ErrorMessage className={css.errorMessage}>{submitError}</ErrorMessage>}
<Box display="flex" flexDirection="row" justifyContent="space-between" gap={3}>
<Button
data-testid="back-btn"
Expand All @@ -264,7 +294,7 @@ const ReviewStep = ({ data, onSubmit, onBack, setStep }: StepRenderProps<NewSafe
size="stretched"
disabled={isDisabled}
>
Next
Create
</Button>
</Box>
</Box>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,7 @@
text-decoration: line-through;
color: var(--color-text-secondary);
}

.errorMessage {
margin-top: 0;
}
Loading
Loading