-
Notifications
You must be signed in to change notification settings - Fork 473
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
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
4be39b1
feat: Create counterfactual 1/1 safes
usame-algan db2b7f0
fix: Add feature flag
usame-algan f9d008d
fix: Lint issues
usame-algan 93b1229
fix: Use incremental saltNonce for all safe creations
usame-algan d84138c
fix: Replace useCounterfactualBalance hook with get function and writ…
usame-algan e26baf1
refactor: Move creation logic out of Review component
usame-algan fdf49f3
fix: useLoadBalance check for undefined value
usame-algan 300f4fd
fix: Extract saltNonce, safeAddress calculation into a hook
usame-algan f49d3c7
refactor: Rename redux slice
usame-algan e040a48
fix: Show error message in case saltNonce can't be retrieved
usame-algan 7b3b385
fix: Disable create button if deploy props are loading
usame-algan ee9496f
fix: Revert hook change and update comment
usame-algan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) | ||
}) | ||
}) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) | ||
|
||
// 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! | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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' | ||
|
@@ -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) | ||
|
@@ -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) | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -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't have enough funds to execute this transaction</ErrorMessage> | ||
)} | ||
</Box> | ||
{isWrongChain && <NetworkWarning />} | ||
|
||
{!walletCanPay && !willRelay && ( | ||
<ErrorMessage> | ||
Your connected wallet doesn'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" | ||
|
@@ -264,7 +294,7 @@ const ReviewStep = ({ data, onSubmit, onBack, setStep }: StepRenderProps<NewSafe | |
size="stretched" | ||
disabled={isDisabled} | ||
> | ||
Next | ||
Create | ||
</Button> | ||
</Box> | ||
</Box> | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,3 +9,7 @@ | |
text-decoration: line-through; | ||
color: var(--color-text-secondary); | ||
} | ||
|
||
.errorMessage { | ||
margin-top: 0; | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Could we not run into Rate Limit issues here? If someone deployed e.g. 10-20 Safes already?
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 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.
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.
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.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.
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.
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 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
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.
We could also add a fallback to the readonly provider in case the wallet provider fails to increase the chances of success.