From 144841c3ba7cf02763dff36627f899a3b7885c07 Mon Sep 17 00:00:00 2001 From: Dmytro Stebaiev Date: Mon, 18 Sep 2023 15:23:04 +0300 Subject: [PATCH] Stop using magic numbers --- .eslintrc.cjs | 2 - src/contractFactory.ts | 14 ++- src/deploy.ts | 49 +++++----- src/gnosis-safe.ts | 21 ++-- src/multiSend.ts | 98 ++++++++----------- src/submitters/auto-submitter.ts | 43 ++++---- .../safe-ima-legacy-marionette-submitter.ts | 6 +- .../safe-ima-marionette-submitter.ts | 3 +- src/submitters/safe-to-ima-submitter.ts | 3 +- src/submitters/submitter.ts | 3 +- src/upgrader.ts | 6 +- src/verification.ts | 9 +- 12 files changed, 130 insertions(+), 127 deletions(-) diff --git a/.eslintrc.cjs b/.eslintrc.cjs index 5755b5b..0b27c52 100644 --- a/.eslintrc.cjs +++ b/.eslintrc.cjs @@ -28,8 +28,6 @@ module.exports = { "never" ], - "no-inline-comments": "warn", - "no-magic-numbers": "warn", "no-mixed-operators": "warn", "no-negated-condition": "warn", "no-shadow": "warn", diff --git a/src/contractFactory.ts b/src/contractFactory.ts index f7b27d0..b323989 100644 --- a/src/contractFactory.ts +++ b/src/contractFactory.ts @@ -62,12 +62,13 @@ const updateManifest = async ( libraries, oldLibraries ); + const indentation = 4; await fs.writeFile( await getManifestFile(), JSON.stringify( manifest, null, - 4 + indentation ) ); }; @@ -99,9 +100,14 @@ export const getContractFactoryAndUpdateManifest = async (contract: string) => { ); }; -const getLibrariesNames = - (linkReferences: LinkReferences) => Object.values(linkReferences). - map((libraryObject) => Object.keys(libraryObject)[0]); + +export const getLibrariesNames = (linkReferences: LinkReferences) => { + const libraryNames = []; + for (const libraryFile of Object.values(linkReferences)) { + libraryNames.push(...Object.keys(libraryFile)); + } + return libraryNames; +}; const getLibrariesToUpgrade = async ( diff --git a/src/deploy.ts b/src/deploy.ts index 30daa83..91fc8eb 100644 --- a/src/deploy.ts +++ b/src/deploy.ts @@ -2,7 +2,9 @@ import {Manifest, hashBytecode} from "@openzeppelin/upgrades-core"; import {artifacts, ethers} from "hardhat"; import {promises as fs} from "fs"; import {SkaleManifestData} from "./types/SkaleManifestData"; -import {Artifact, LinkReferences} from "hardhat/types"; +import {Artifact} from "hardhat/types"; +import {hexDataSlice, hexConcat} from "ethers/lib/utils"; +import {getLibrariesNames} from "./contractFactory"; interface LibraryArtifacts { [key: string]: unknown @@ -39,23 +41,32 @@ export const deployLibraries = async (libraryNames: string[]) => { return libraries; }; -const _linkBytecode = (artifact: Artifact, libraries: Map) => { +const firstByteIndex = 0; + +const linkBytecode = (artifact: Artifact, libraries: Map) => { let {bytecode} = artifact; for (const [, fileReferences] of Object.entries(artifact.linkReferences)) { for (const [ libName, fixups ] of Object.entries(fileReferences)) { - const addr = libraries.get(libName); - if (addr !== undefined) { + const libAddress = libraries.get(libName); + if (typeof libAddress !== "undefined") { for (const fixup of fixups) { - bytecode = - bytecode.substr( - 0, - 2 + fixup.start * 2 - ) + - addr.substr(2) + - bytecode.substr(2 + (fixup.start + fixup.length) * 2); + const bytecodeBefore = hexDataSlice( + bytecode, + firstByteIndex, + fixup.start + ); + const bytecodeAfter = hexDataSlice( + bytecode, + fixup.start + fixup.length + ); + bytecode = hexConcat([ + bytecodeBefore, + libAddress, + bytecodeAfter + ]); } } } @@ -69,7 +80,7 @@ export const getLinkedContractFactory = async ( ) => { const cArtifact = await artifacts.readArtifact(contractName); - const linkedBytecode = _linkBytecode( + const linkedBytecode = linkBytecode( cArtifact, libraries ); @@ -100,12 +111,13 @@ const updateManifest = async (libraryArtifacts: LibraryArtifacts) => { manifest.libraries ); } + const indentation = 4; await fs.writeFile( await getManifestFile(), JSON.stringify( manifest, null, - 4 + indentation ) ); }; @@ -142,22 +154,13 @@ const getLibraryArtifacts = async (libraries: Map) => { return libraryArtifacts; }; -const getLibraryNames = (linkReferences: LinkReferences) => { - const libraryNames = []; - for (const key of Object.keys(linkReferences)) { - const libraryName = Object.keys(linkReferences[key])[0]; - libraryNames.push(libraryName); - } - return libraryNames; -}; - export const getContractFactory = async (contract: string) => { const {linkReferences} = await artifacts.readArtifact(contract); if (!Object.keys(linkReferences).length) { return await ethers.getContractFactory(contract); } - const libraryNames = getLibraryNames(linkReferences); + const libraryNames = getLibrariesNames(linkReferences); const libraries = await deployLibraries(libraryNames); const libraryArtifacts = await getLibraryArtifacts(libraries); diff --git a/src/gnosis-safe.ts b/src/gnosis-safe.ts index c19cba6..43360d4 100644 --- a/src/gnosis-safe.ts +++ b/src/gnosis-safe.ts @@ -5,24 +5,21 @@ import SafeApiKit from "@safe-global/api-kit"; import Safe, {EthersAdapter} from "@safe-global/protocol-kit"; import { MetaTransactionData, + OperationType, SafeTransaction, SafeTransactionDataPartial } from "@safe-global/safe-core-sdk-types"; +import {getNetwork} from "@ethersproject/networks"; -enum Network { - MAINNET = 1, - GOERLI = 5, - GANACHE = 1337, - HARDHAT = 31337, -} - // Constants const URLS = { "safe_transaction": { - [Network.MAINNET]: "https://safe-transaction-mainnet.safe.global", - [Network.GOERLI]: "https://safe-transaction-goerli.safe.global" + [getNetwork("mainnet").chainId]: + "https://safe-transaction-mainnet.safe.global", + [getNetwork("goerli").chainId]: + "https://safe-transaction-goerli.safe.global" } }; @@ -129,10 +126,10 @@ const estimateSafeTransaction = async ( map((transaction) => safeService.estimateSafeTransaction( safeAddress, { - "to": transaction.to, - "value": transaction.value, "data": transaction.data, - "operation": transaction.operation || 0 + "operation": transaction.operation || OperationType.Call, + "to": transaction.to, + "value": transaction.value } ))); for (const estimateResponse of gasEstimations) { diff --git a/src/multiSend.ts b/src/multiSend.ts index 1c41073..54b41ca 100644 --- a/src/multiSend.ts +++ b/src/multiSend.ts @@ -1,30 +1,12 @@ -import {BigNumber} from "ethers"; - -const padWithZeros = ( - value: string, - targetLength: number -) => ("0".repeat(targetLength) + value).slice(-targetLength); - -const getOperationBytes = (operation: 0 | 1) => { - if (operation === 0) { - return "00"; - } else if (operation === 1) { - return "01"; - } - throw Error("Operation has an incorrect value"); -}; - -const getToBytes = (to: string) => { - let _to = to; - if (to.startsWith("0x")) { - _to = _to.slice(2); - } - _to = padWithZeros( - _to, - 20 * 2 - ); - return _to; -}; +import {OperationType} from "@safe-global/safe-core-sdk-types"; +import {BigNumberish, BytesLike} from "ethers"; +import { + hexConcat, + hexDataLength, + hexValue, + hexZeroPad, + hexlify +} from "ethers/lib/utils"; interface Transaction { @@ -32,47 +14,49 @@ interface Transaction { * Operation as a uint8 with 0 for a call * or 1 for a delegatecall (=> 1 byte) */ - operation: 0 | 1, + operation: OperationType, // To as a address (=> 20 bytes) to: string, // Value as a uint256 (=> 32 bytes) - value: BigNumber | number, + value: BigNumberish, // Data as bytes. - data: string + data: BytesLike } -export const encodeTransaction = (transaction: Transaction) => { - const _operation = getOperationBytes(transaction.operation); +const OPERATION_BYTES = 1; +const ADDRESS_BYTES = 20; +const UINT256_BYTES = 32; +const TO_BYTES = ADDRESS_BYTES; +const VALUE_BYTES = UINT256_BYTES; +const DATA_LENGTH_BYTES = UINT256_BYTES; - const _to = getToBytes(transaction.to); - - const _value = padWithZeros( - BigNumber.from(transaction.value).toHexString(). - slice(2), - 32 * 2 +export const encodeTransaction = (transaction: Transaction) => { + const operation = hexZeroPad( + hexValue(transaction.operation), + OPERATION_BYTES ); - - let _data = transaction.data; - if (transaction.data.startsWith("0x")) { - _data = _data.slice(2); - } - if (_data.length % 2 !== 0) { - _data = `0${_data}`; - } - - const _dataLength = padWithZeros( - (_data.length / 2).toString(16), - 32 * 2 + const to = hexZeroPad( + hexValue(transaction.to), + TO_BYTES + ); + const value = hexZeroPad( + hexValue(transaction.value), + VALUE_BYTES + ); + const data = hexlify(transaction.data); + const dataLength = hexZeroPad( + hexValue(hexDataLength(data)), + DATA_LENGTH_BYTES ); - return `0x${[ - _operation, - _to, - _value, - _dataLength, - _data - ].join("")}`; + return hexConcat([ + operation, + to, + value, + dataLength, + data + ]); }; diff --git a/src/submitters/auto-submitter.ts b/src/submitters/auto-submitter.ts index 818fa46..5d79cfa 100644 --- a/src/submitters/auto-submitter.ts +++ b/src/submitters/auto-submitter.ts @@ -11,10 +11,27 @@ import { } from "./safe-ima-legacy-marionette-submitter"; import {MARIONETTE_ADDRESS} from "./types/marionette"; import {skaleContracts} from "@skalenetwork/skale-contracts-ethers-v5"; +import {EXIT_CODES} from "../exitCodes"; export class AutoSubmitter extends Submitter { name = "Auto Submitter"; + static marionetteInterface = [ + { + "inputs": [], + "name": "version", + "outputs": [ + { + "internalType": "string", + "name": "", + "type": "string" + } + ], + "stateMutability": "view", + "type": "function" + } + ]; + async submit (transactions: Transaction[]) { console.log(`Submit via ${this.name}`); const submitter = await AutoSubmitter.getSubmitter(); @@ -91,7 +108,7 @@ export class AutoSubmitter extends Submitter { if (!process.env.IMA) { console.log(chalk.red("Set target IMA alias" + " to IMA environment variable")); - process.exit(1); + process.exit(EXIT_CODES.UNKNOWN_IMA); } const network = await skaleContracts.getNetworkByProvider(ethers.provider); @@ -103,7 +120,7 @@ export class AutoSubmitter extends Submitter { if (!process.env.SAFE_ADDRESS) { console.log(chalk.red("Set Gnosis Safe owner address" + " to SAFE_ADDRESS environment variable")); - process.exit(1); + process.exit(EXIT_CODES.UNKNOWN_SAFE_ADDRESS); } return process.env.SAFE_ADDRESS; } @@ -142,35 +159,23 @@ export class AutoSubmitter extends Submitter { private static async _versionFunctionExists () { const bytecode = await hre.ethers.provider.getCode(MARIONETTE_ADDRESS); + const hexPrefixLength = 2; + const selectorLength = 10; /* * If the bytecode doesn't include the function selector version() * is definitely not present */ if (!bytecode.includes(ethers.utils.id("version()").slice( - 2, - 10 + hexPrefixLength, + selectorLength ))) { return false; } const marionette = new ethers.Contract( MARIONETTE_ADDRESS, - [ - { - "inputs": [], - "name": "version", - "outputs": [ - { - "internalType": "string", - "name": "", - "type": "string" - } - ], - "stateMutability": "view", - "type": "function" - } - ], + AutoSubmitter.marionetteInterface, hre.ethers.provider ); diff --git a/src/submitters/safe-ima-legacy-marionette-submitter.ts b/src/submitters/safe-ima-legacy-marionette-submitter.ts index 7b8fa42..29ca59d 100644 --- a/src/submitters/safe-ima-legacy-marionette-submitter.ts +++ b/src/submitters/safe-ima-legacy-marionette-submitter.ts @@ -41,9 +41,11 @@ export class SafeImaLegacyMarionetteSubmitter extends SafeToImaSubmitter { ); async submit (transactions: UnsignedTransaction[]): Promise { - if (transactions.length > 1) { + const singleTransaction = 1; + if (transactions.length > singleTransaction) { SafeImaLegacyMarionetteSubmitter._atomicityWarning(); } + const zeroValue = 0; const transactionsToMarionette = (await Promise.all(transactions. map((transaction) => this.marionette.encodeFunctionCall( @@ -52,7 +54,7 @@ export class SafeImaLegacyMarionetteSubmitter extends SafeToImaSubmitter { : ethers.constants.AddressZero, transaction.value ? transaction.value - : 0, + : zeroValue, transaction.data ? transaction.data : "0x" diff --git a/src/submitters/safe-ima-marionette-submitter.ts b/src/submitters/safe-ima-marionette-submitter.ts index 218404f..5f5b3e4 100644 --- a/src/submitters/safe-ima-marionette-submitter.ts +++ b/src/submitters/safe-ima-marionette-submitter.ts @@ -49,6 +49,7 @@ export class SafeImaMarionetteSubmitter extends SafeToImaSubmitter { async submit (transactions: UnsignedTransaction[]): Promise { const functionCalls = []; + const zeroValue = 0; for (const transaction of transactions) { functionCalls.push({ "receiver": transaction.to @@ -56,7 +57,7 @@ export class SafeImaMarionetteSubmitter extends SafeToImaSubmitter { : ethers.constants.AddressZero, "value": transaction.value ? transaction.value - : 0, + : zeroValue, "data": transaction.data ? transaction.data : "0x" diff --git a/src/submitters/safe-to-ima-submitter.ts b/src/submitters/safe-to-ima-submitter.ts index 380d341..66d5f75 100644 --- a/src/submitters/safe-to-ima-submitter.ts +++ b/src/submitters/safe-to-ima-submitter.ts @@ -28,7 +28,8 @@ export class SafeToImaSubmitter extends SafeSubmitter { } async submit (transactions: UnsignedTransaction[]): Promise { - if (transactions.length > 1) { + const singleTransaction = 1; + if (transactions.length > singleTransaction) { SafeToImaSubmitter._atomicityWarning(); } const messageProxyForMainnet = await this._getMessageProxyForMainnet(); diff --git a/src/submitters/submitter.ts b/src/submitters/submitter.ts index 3ca3b9f..6de9e55 100644 --- a/src/submitters/submitter.ts +++ b/src/submitters/submitter.ts @@ -1,5 +1,6 @@ import {UnsignedTransaction} from "ethers"; import chalk from "chalk"; +import {EXIT_CODES} from "../exitCodes"; export abstract class Submitter { abstract submit(transactions: UnsignedTransaction[]): Promise; @@ -12,7 +13,7 @@ export abstract class Submitter { " of multiple transactions and will not be atomic")); console.log(chalk.red("If not atomic upgrade is OK" + " set ALLOW_NOT_ATOMIC_UPGRADE environment variable")); - process.exit(1); + process.exit(EXIT_CODES.NOT_ATOMIC_UPGRADE); } else { console.log(chalk.yellow("Not atomic upgrade is performing")); } diff --git a/src/upgrader.ts b/src/upgrader.ts index db9799c..95115bc 100644 --- a/src/upgrader.ts +++ b/src/upgrader.ts @@ -11,6 +11,7 @@ import {Submitter} from "./submitters/submitter"; import {AutoSubmitter} from "./submitters/auto-submitter"; import {Instance} from "@skalenetwork/skale-contracts-ethers-v5"; import {getContractFactoryAndUpdateManifest} from "./contractFactory"; +import {EXIT_CODES} from "./exitCodes"; interface ContractToUpgrade { @@ -125,12 +126,13 @@ export abstract class Upgrader { } private async writeTransactions (version: string) { + const indentation = 4; await fs.writeFile( `data/transactions-${version}-${network.name}.json`, JSON.stringify( this.transactions, null, - 4 + indentation ) ); } @@ -227,7 +229,7 @@ export abstract class Upgrader { `This script can't upgrade version ${deployedVersion}` + ` to ${version}`; console.log(chalk.red(cannotUpgradeMessage)); - process.exit(1); + process.exit(EXIT_CODES.BAD_VERSION); } } else { const cannotCheckMessage = diff --git a/src/verification.ts b/src/verification.ts index 5a79f03..5d36721 100644 --- a/src/verification.ts +++ b/src/verification.ts @@ -5,6 +5,8 @@ import { import chalk from "chalk"; import {getImplementationAddress} from "@openzeppelin/upgrades-core"; +const RETRIES_AMOUNT = 5; + const processError = (error: unknown, contractName: string) => { if (error instanceof Error) { const alreadyVerifiedErrorLine = @@ -59,15 +61,16 @@ const verifyWithRetry = async ( verificationTarget: VerificationTarget, attempts: number ) => { - if (attempts > 0) { + if (attempts) { if (!await verificationAttempt( verificationTarget.contractName, verificationTarget.contractAddress, verificationTarget.constructorArguments )) { + const failedAttempts = 1; await verifyWithRetry( verificationTarget, - attempts - 1 + attempts - failedAttempts ); } } @@ -90,7 +93,7 @@ export const verify = async ( contractAddress, constructorArguments }, - 5 + RETRIES_AMOUNT ); };