Skip to content

Commit

Permalink
fix(core): remove CREATE3 proxies from preview warning
Browse files Browse the repository at this point in the history
  • Loading branch information
sam-goldman committed Jan 16, 2024
1 parent b802adf commit c8fc9e0
Show file tree
Hide file tree
Showing 10 changed files with 95 additions and 43 deletions.
6 changes: 6 additions & 0 deletions .changeset/ninety-ties-return.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@sphinx-labs/plugins': patch
'@sphinx-labs/core': patch
---

Remove `CREATE3` proxies from preview warning
9 changes: 6 additions & 3 deletions docs/deployment-artifacts.md
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,10 @@ type ExecutionArtifact = {
isModuleDeployed: boolean
isExecuting: boolean
}
unlabeledAddresses: Array<string>
unlabeledContracts: Array<{
address: string
initCodeWithArgs: string
}>
arbitraryChain: boolean
libraries: Array<string>
gitCommit: string | null
Expand All @@ -343,7 +346,7 @@ type ExecutionArtifact = {
* **sphinxConfig**: The configuration options in the deployment script.
* **executionMode**: Whether the deployment was executed on a local network via the CLI (`0`), on a live network via the CLI (`1`), or via the DevOps Platform (`2`).
* **initialState**: On-chain state variables that were recorded before the deployment was executed.
* **unlabeledAddresses**: An array of contract addresses in the deployment that weren't labeled. These contracts won't be verified on block explorers, and Sphinx will not create a deployment artifact for them. If a contract does not have a source file, it will be in this array. Common examples are minimal `CREATE3` or `EIP-1167` proxies.
* **unlabeledContracts**: An array of contracts Sphinx couldn't find an artifact for. These contracts won't be verified on block explorers, and Sphinx will not create a deployment artifact for them. If a contract does not have a source file, it will be in this array. Common examples are minimal `CREATE3` or `EIP-1167` proxies.
* **arbitraryChain**: Indicates whether the deployment can be executed on an arbitrary chain. Currently always false.
* **libraries**: An array of libraries that were used in the deployment. These are in the format that the Solidity compiler expects. For example, `path/to/file.sol:MyLibrary=0x1234567890123456789012345678901234567890`.
* **gitCommit**: The full git commit hash on the machine that initiated the deployment. If the deployment was executed via the DevOps Platform, this is recorded on the machine that proposed the deployment. If the deployment was executed from the user's local machine instead of the DevOps Platform, this is recorded on the user's machine when they run the `deploy` CLI command. This is null if the repository was not a git repository when the deployment was initiated.
Expand Down Expand Up @@ -450,7 +453,7 @@ type ExecutionArtifact = {
"isModuleDeployed": false,
"isExecuting": false
},
"unlabeledAddresses": [],
"unlabeledContracts": [],
"arbitraryChain": false,
"libraries": [],
"gitCommit": "88a023502161a4be85b4b4340e1066c03f60ce54",
Expand Down
17 changes: 13 additions & 4 deletions packages/core/src/artifacts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,7 @@ const makeExecutionArtifact = async (
chainId,
actionInputs,
executionMode,
unlabeledAddresses,
unlabeledContracts,
arbitraryChain,
libraries,
gitCommit,
Expand Down Expand Up @@ -684,7 +684,7 @@ const makeExecutionArtifact = async (
isModuleDeployed,
isExecuting,
},
unlabeledAddresses,
unlabeledContracts,
arbitraryChain,
libraries,
gitCommit,
Expand Down Expand Up @@ -721,6 +721,16 @@ const isLibraryArray = (ary: any): boolean => {
})
}

const isUnlabeledContracts = (ary: any): boolean => {
return (
Array.isArray(ary) &&
ary.every(
({ address, initCodeWithArgs }) =>
ethers.isAddress(address) && typeof initCodeWithArgs === 'string'
)
)
}

export const isExecutionArtifact = (obj: any): obj is ExecutionArtifact => {
return (
obj !== null &&
Expand Down Expand Up @@ -760,8 +770,7 @@ export const isExecutionArtifact = (obj: any): obj is ExecutionArtifact => {
typeof obj.initialState.isSafeDeployed === 'boolean' &&
typeof obj.initialState.isModuleDeployed === 'boolean' &&
typeof obj.initialState.isExecuting === 'boolean' &&
Array.isArray(obj.unlabeledAddresses) &&
obj.unlabeledAddresses.every((addr) => typeof addr === 'string') &&
isUnlabeledContracts(obj.unlabeledContracts) &&
typeof obj.arbitraryChain === 'boolean' &&
isLibraryArray(obj.libraries) &&
(typeof obj.gitCommit === 'string' || obj.gitCommit === null) &&
Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/config/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,10 @@ export type ParsedConfig = {
executionMode: ExecutionMode
initialState: InitialChainState
isSystemDeployed: boolean
unlabeledAddresses: Array<string>
unlabeledContracts: Array<{
address: string
initCodeWithArgs: string
}>
arbitraryChain: boolean
libraries: Array<string>
gitCommit: string | null
Expand Down
13 changes: 8 additions & 5 deletions packages/core/src/languages/solidity/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ import { ExecutionMode } from '../../constants'
* via the CLI, on a live network via the CLI, or via the DevOps Platform.
* @property {Object} initialState - On-chain state variables that were recorded before the
* deployment was executed.
* @property {Array<string>} unlabeledAddresses - An array of contract addresses in the deployment
* that weren't labeled. These contracts won't be verified on block explorers, and Sphinx will not
* create a deployment artifact for them. If a contract does not have a source file, it will be in
* this array. Common examples are minimal `CREATE3` or `EIP-1167` proxies.
* @property {Array<Object>} unlabeledContracts - An array of contracts Sphinx couldn't find an
* artifact for. These contracts won't be verified on block explorers, and Sphinx will not create a
* deployment artifact for them. If a contract does not have a source file, it will be in this
* array. Common examples are minimal `CREATE3` or `EIP-1167` proxies.
* @property {boolean} arbitraryChain - Indicates whether the deployment can be executed on an
* arbitrary chain. Currently always false.
* @property {Array<string>} libraries - An array of libraries that were used in the deployment.
Expand Down Expand Up @@ -81,7 +81,10 @@ export type ExecutionArtifact = {
isModuleDeployed: boolean
isExecuting: boolean
}
unlabeledAddresses: Array<string>
unlabeledContracts: Array<{
address: string
initCodeWithArgs: string
}>
arbitraryChain: boolean
libraries: Array<string>
gitCommit: string | null
Expand Down
19 changes: 18 additions & 1 deletion packages/core/src/preview.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { yellow, green, bold } from 'chalk'
import { CREATE3_PROXY_INITCODE } from '@sphinx-labs/contracts'

import { DecodedAction, ParsedConfig } from './config/types'
import {
Expand All @@ -18,6 +19,15 @@ type PreviewElement =
| { to: string; data: string }
| SystemDeploymentElement

/**
* @property unlabeledAddresses A set of unlabeled addresses. The preview will warn the user that
* these addresses will not be verified on Etherscan and they will not have a deployment artifact.
* This set does not include any `CREATE3` proxies even though they're unlabeled. We exclude
* `CREATE3` proxies because we assume that the user doesn't need a contract deployment artifact for
* them, since users never interact directly with these proxies. Also, if a user isn't aware that
* `CREATE3` involves a proxy deployment, they may reasonably be confused about a warning for a
* contract they didn't know existed.
*/
export type SphinxPreview = {
networks: Array<{
networkTags: Array<string>
Expand Down Expand Up @@ -166,10 +176,17 @@ export const getPreview = (
initialState,
actionInputs,
executionMode,
unlabeledAddresses,
unlabeledContracts,
isSystemDeployed,
} = parsedConfig

const unlabeledAddresses = unlabeledContracts
// Remove the `CREATE3` proxies.
.filter(
(contract) => contract.initCodeWithArgs !== CREATE3_PROXY_INITCODE
)
.map((contract) => contract.address)

const networkName = getNetworkNameForChainId(BigInt(chainId))
const networkTag = getNetworkTag(
networkName,
Expand Down
45 changes: 25 additions & 20 deletions packages/core/test/preview.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { expect } from 'chai'
import { ethers } from 'ethers'
import { Operation } from '@sphinx-labs/contracts'
import { CREATE3_PROXY_INITCODE, Operation } from '@sphinx-labs/contracts'

import { Create2ActionInput, ParsedConfig } from '../src/config/types'
import { SphinxActionType } from '../src/actions/types'
Expand Down Expand Up @@ -88,11 +88,23 @@ const expectedCall: FunctionCallActionInput = {
additionalContracts: [],
gas: '0',
}

const unlabeledAddressOne = '0x' + '55'.repeat(20)
const unlabeledAddressTwo = '0x' + '66'.repeat(20)
const originalParsedConfig: ParsedConfig = {
chainId: '10',
executionMode: ExecutionMode.Platform,
actionInputs: [expectedCreate2, expectedFunctionCallOne, expectedCall],
unlabeledAddresses: ['0x' + '55'.repeat(20), '0x' + '66'.repeat(20)],
unlabeledContracts: [
{ address: unlabeledAddressOne, initCodeWithArgs: '0x123456' },
{ address: unlabeledAddressTwo, initCodeWithArgs: '0x7890' },
// We include a `CREATE3` proxy to test that the preview doesn't include it in the
// `unlabeledAddresses` set.
{
address: '0x' + '77'.repeat(20),
initCodeWithArgs: CREATE3_PROXY_INITCODE,
},
],
safeAddress: '0x' + 'ff'.repeat(20),
moduleAddress: '0x' + 'ee'.repeat(20),
initialState: {
Expand Down Expand Up @@ -122,6 +134,11 @@ const originalParsedConfig: ParsedConfig = {
}

describe('Preview', () => {
const expectedUnlabeledAddresses = new Set([
unlabeledAddressOne,
unlabeledAddressTwo,
])

describe('getPreview', () => {
it('returns preview for single network that is executing everything, including Gnosis Safe and Sphinx Module', () => {
const { networks, unlabeledAddresses } = getPreview([
Expand All @@ -139,9 +156,7 @@ describe('Preview', () => {
expect(functionCall).to.deep.equal(expectedFunctionCallOne.decodedAction)
expect(call).to.deep.equal(expectedCall.decodedAction)
expect(skipping.length).to.equal(0)
expect(unlabeledAddresses).to.deep.equal(
new Set(originalParsedConfig.unlabeledAddresses)
)
expect(unlabeledAddresses).to.deep.equal(expectedUnlabeledAddresses)
})

it('returns preview for single network that is executing everything, except Gnosis Safe and Sphinx Module', () => {
Expand All @@ -160,9 +175,7 @@ describe('Preview', () => {
expect(functionCall).to.deep.equal(expectedFunctionCallOne.decodedAction)
expect(call).to.deep.equal(expectedCall.decodedAction)
expect(skipping.length).to.equal(0)
expect(unlabeledAddresses).to.deep.equal(
new Set(originalParsedConfig.unlabeledAddresses)
)
expect(unlabeledAddresses).to.deep.equal(expectedUnlabeledAddresses)
})

// If a function call or constructor has at least one unnamed argument, then the arguments will be
Expand Down Expand Up @@ -200,9 +213,7 @@ describe('Preview', () => {
})
expect(call).to.deep.equal(expectedCall.decodedAction)
expect(skipping.length).to.equal(0)
expect(unlabeledAddresses).to.deep.equal(
new Set(originalParsedConfig.unlabeledAddresses)
)
expect(unlabeledAddresses).to.deep.equal(expectedUnlabeledAddresses)
})

it('returns merged preview for networks that are the same', () => {
Expand Down Expand Up @@ -236,9 +247,7 @@ describe('Preview', () => {
expectedFunctionCallOne.decodedAction
)
expect(callOptimism).to.deep.equal(expectedCall.decodedAction)
expect(unlabeledAddresses).to.deep.equal(
new Set(originalParsedConfig.unlabeledAddresses)
)
expect(unlabeledAddresses).to.deep.equal(expectedUnlabeledAddresses)
})

it('returns preview for networks that are different', () => {
Expand Down Expand Up @@ -333,9 +342,7 @@ describe('Preview', () => {
)
expect(callArbitrum).to.deep.equal(expectedCall.decodedAction)
expect(skippingArbitrum.length).to.equal(0)
expect(unlabeledAddresses).to.deep.equal(
new Set(originalParsedConfig.unlabeledAddresses)
)
expect(unlabeledAddresses).to.deep.equal(expectedUnlabeledAddresses)
})

it('returns preview when `isSystemDeployed` is `false`', () => {
Expand Down Expand Up @@ -368,9 +375,7 @@ describe('Preview', () => {
expect(functionCall).to.deep.equal(expectedFunctionCallOne.decodedAction)
expect(call).to.deep.equal(expectedCall.decodedAction)
expect(skipping.length).to.equal(0)
expect(unlabeledAddresses).to.deep.equal(
new Set(originalParsedConfig.unlabeledAddresses)
)
expect(unlabeledAddresses).to.deep.equal(expectedUnlabeledAddresses)
})
})
})
17 changes: 10 additions & 7 deletions packages/plugins/src/foundry/decode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ export const makeParsedConfig = (
const maxAllowedGasPerLeaf = (BigInt(8) * BigInt(blockGasLimit)) / BigInt(10)

const parsedActionInputs: Array<ActionInput> = []
const unlabeledAddresses: Array<string> = []
const unlabeledContracts: ParsedConfig['unlabeledContracts'] = []
// We start with an action index of 1 because the `APPROVE` leaf always has an index of 0, which
// means the `EXECUTE` leaves start with an index of 1.
let actionIndex = 1
Expand All @@ -275,7 +275,7 @@ export const makeParsedConfig = (

const { parsedContracts, unlabeledAdditionalContracts } =
parseAdditionalContracts(input, configArtifacts)
unlabeledAddresses.push(...unlabeledAdditionalContracts)
unlabeledContracts.push(...unlabeledAdditionalContracts)

if (isRawCreate2ActionInput(input)) {
const fullyQualifiedName = findFullyQualifiedName(
Expand All @@ -294,7 +294,10 @@ export const makeParsedConfig = (
} else {
// We couldn't find the fully qualified name, so the contract must not belong to a source
// file. We mark it as unlabeled.
unlabeledAddresses.push(input.create2Address)
unlabeledContracts.push({
address: input.create2Address,
initCodeWithArgs: input.initCodeWithArgs,
})
}

parsedActionInputs.push({
Expand Down Expand Up @@ -330,7 +333,7 @@ export const makeParsedConfig = (
initialState,
isSystemDeployed,
actionInputs: parsedActionInputs,
unlabeledAddresses,
unlabeledContracts,
arbitraryChain,
executorAddress: deploymentInfo.executorAddress,
libraries: convertLibraryFormat(libraries),
Expand All @@ -350,10 +353,10 @@ const parseAdditionalContracts = (
configArtifacts: ConfigArtifacts
): {
parsedContracts: Array<ParsedContractDeployment>
unlabeledAdditionalContracts: Array<string>
unlabeledAdditionalContracts: ParsedConfig['unlabeledContracts']
} => {
const parsedContracts: Array<ParsedContractDeployment> = []
const unlabeled: Array<string> = []
const unlabeled: ParsedConfig['unlabeledContracts'] = []
for (const additionalContract of currentInput.additionalContracts) {
const address = ethers.getAddress(additionalContract.address)

Expand All @@ -368,7 +371,7 @@ const parseAdditionalContracts = (
initCodeWithArgs: additionalContract.initCode,
})
} else {
unlabeled.push(address)
unlabeled.push({ address, initCodeWithArgs: additionalContract.initCode })
}
}

Expand Down
5 changes: 4 additions & 1 deletion packages/plugins/test/mocha/cli/deploy.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,10 @@ describe('Deployment Cases', () => {
}

const checkNotLabeled = (address: string) => {
expect(compilerConfig!.unlabeledAddresses.includes(address)).to.eq(true)
const isAddressUnlabeled = compilerConfig!.unlabeledContracts.some(
(contract) => contract.address === address
)
expect(isAddressUnlabeled).to.eq(true)
}

before(async () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/plugins/test/mocha/mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ const makeMockParsedConfig = (): ParsedConfig => {
isExecuting: false,
},
isSystemDeployed: true,
unlabeledAddresses: [],
unlabeledContracts: [],
arbitraryChain: false,
libraries: [],
gitCommit: null,
Expand Down

0 comments on commit c8fc9e0

Please sign in to comment.