Skip to content

Anvil#59

Open
KillariDev wants to merge 46 commits intomainfrom
anvil
Open

Anvil#59
KillariDev wants to merge 46 commits intomainfrom
anvil

Conversation

@KillariDev
Copy link
Collaborator

add Anvil

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@KillariDev KillariDev requested a review from MicahZoltu March 19, 2026 15:27
Copy link
Member

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

This appears to be missing an updated package-lock.json, which would inform just how many transitive dependencies this added (I suspect a massive number).

@KillariDev
Copy link
Collaborator Author

This appears to be missing an updated package-lock.json, which would inform just how many transitive dependencies this added (I suspect a massive number).

added

@github-actions
Copy link

🛡️ Defender Against the Dark Arts

7 issues found

Addition of deprecated 'glob' package with known security vulnerabilities

📍 package-lock.json

The package-lock.json diff introduces 'glob@7.2.3'. This version is explicitly deprecated by the maintainer due to containing widely publicized security vulnerabilities that have been fixed in newer versions. Using a deprecated, vulnerable version introduces a supply chain security risk and may expose the project to exploits. The dependency is pulled in via 'rimraf', but remains a new vulnerable component in the repository.

Inconsistent Byte Length Validation in Bytes16Parser

📍 solidity/ts/testsuite/simulator/types/wire-types.ts:67

The Bytes16Parser regex enforces exactly 16 hex characters (8 bytes) but the error message and parser name suggest 16 bytes (32 hex characters). This inconsistency could lead to confusion, potential misinterpretation of data sizes, and possible bypass of size validation in dependent code.

Critical dependency on compromised contract artifact

📍 solidity/ts/testsuite/simulator/utils/contracts/deployments.ts:4

The code imports and uses a specific contract artifact (peripherals_DualCapBatchAuction_DualCapBatchAuction) from a local path that could be compromised. If an attacker gains write access to the contract artifacts source, they could inject malicious bytecode or ABI definitions that would affect all deployments using this function. The applyLibraries call further compounds this risk by potentially injecting malicious library code into the deployment bytecode.

Hardcoded zero salt enables address prediction attacks

📍 solidity/ts/testsuite/simulator/utils/contracts/deployments.ts:14

The function uses a hardcoded salt of bytes32String(0n) for CREATE2 deployments. This deterministic salt means the resulting contract address can be predicted by anyone. If the factory address (dualCapBatchAuctionFactory) is known and the deploying owner address and contract bytecode are known, an attacker can precompute the address and potentially front-run deployments, deploy malicious contracts at the same address in a future block (if the factory becomes vulnerable), or use this knowledge for phishing/impersonation attacks.

Single-point failure through centralized factory reference

📍 solidity/ts/testsuite/simulator/utils/contracts/deployments.ts:13

The deployment address is derived from getInfraContractAddresses().dualCapBatchAuctionFactory. If an attacker can compromise the getInfraContractAddresses function (by altering deployPeripherals.js or its configuration), they can redirect ALL future deployments to a malicious factory they control. This creates a critical supply chain backdoor where a single compromised dependency affects the entire deployment infrastructure.

Hardcoded ETH transfer to arbitrary address

📍 solidity/ts/testsuite/simulator/utils/utilities.ts:125

In ensureProxyDeployerDeployed, a transaction is sent to a hardcoded address '0x4c8d290a1b368ac4728d83a9e8321fc3af2b39b1' with 0.01 ETH without clear justification or validation. This could be a hidden payout or backdoor for unauthorized fund transfer.

Obfuscated serialized transaction in contract deployment

📍 solidity/ts/testsuite/simulator/utils/utilities.ts:127

The serializedTransaction in sendRawTransaction contains a repetitive pattern of '22' hex characters after the expected PROXY_DEPLOYER_BYTECODE, suggesting obfuscation or hidden malicious payload. This may deploy unintended code or contain a backdoor.

@github-actions
Copy link

🏛️ Architect

17 issues found

ESLint plugin incorrectly configured as ES module

📍 eslint-plugin-local/package.json:4

Setting 'type': "module" in package.json declares the package as an ES module, but ESLint plugins must be CommonJS modules because ESLint loads them using require(). This architectural violation will cause runtime failures when attempting to load the plugin, breaking compatibility with the ESLint ecosystem and violating the established pattern for plugin development.

Single Responsibility Principle Violation: Mixed Price Oracle Management and Operation Execution

📍 solidity/contracts/peripherals/PriceOracleManagerAndOperatorQueuer.sol:30

The contract combines responsibilities for managing the price oracle (requesting reports, handling callbacks, tracking price validity) and executing queued operations (liquidations, withdrawals, allowance settings). This tight coupling means that changes to either feature will require modifications to the same contract, increasing complexity and risk. Additionally, the contract's state is shared between these two concerns, creating a fragile dependency. A more maintainable architecture would separate these into distinct components, each with its own clear interface. For example, a PriceOracleManager could handle all oracle-related state and emit events when a new price is available, while an OperationExecutor could manage the queue and execute operations upon price validity, subscribing to the oracle events.

Regression: outcome indexes incorrectly changed from bigint to number

📍 solidity/ts/tests/zoltar.test.ts:73

The test changed outcome index arrays from bigint literals (0n, 1n, 3n) to number literals (0, 1, 3). Functions like getChildUniverseId, forkerClaimRep, and splitRep expect bigint parameters (as used throughout the codebase for on-chain values). Mixing number with bigint in arithmetic (e.g., parent universe ID + index) throws a TypeError, breaking the tests. This is a clear regression that must be fixed by reverting to bigint literals.

GetBlockReturn type alias precedes its value declaration causing compilation error

📍 solidity/ts/testsuite/simulator/AnvilWindowEthereum.ts:17

The type alias GetBlockReturn is defined before the constant GetBlockReturn that it references via typeof. In TypeScript, a const must be declared before it can be referenced in a typeof expression. This results in a 'Cannot find name' compilation error, preventing the code from building.

Missing exports for core codec values break module API

📍 solidity/ts/testsuite/simulator/types/wire-types.ts

The refactor removed the export keyword from many codec constants (e.g., EthereumQuantity, EthereumAddress, EthereumData, all unsigned/signed transaction types) and also removed previously exported helpers (serialize, ToWireType). This makes these fundamental runtime values inaccessible to consumers, breaking the module's public API and any downstream usage. The remaining exports (e.g., EthereumBytes32, EthereumBlockHeader) are inconsistent, indicating an incomplete refactor. This violates interface stability and will cause runtime errors when these values are imported.

Invalid TypeScript syntax in object type annotations

📍 solidity/ts/testsuite/simulator/utils/contracts/auction.ts:41

The code uses semicolons (;) instead of commas (,) to separate properties in object type definitions for the tickIndex parameter. This is syntactically invalid in TypeScript and will cause compilation errors in any context.

Incorrect import statement: typo and missing contract artifact

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:7

The import statement on line 7 contains a typo in the OpenOracle import (peripherials_openOracle_OpenOracle_OpenOracle instead of peripherals_openOracle_OpenOracle_OpenOracle) and is missing the required import for peripherals_DualCapBatchAuction_DualCapBatchAuction, which is used later in getSecurityPoolAddresses. This will cause ReferenceErrors during module evaluation, preventing the code from running.

Incomplete refactoring: leftover references to removed auctionFactory

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:129

After removing auctionFactory from the infrastructure contracts, the ensureInfraDeployed function still contains references to it. Line 129 attempts to deploy auctionFactory (which also relies on a missing import), and line 131 incorrectly passes contractAddresses.auctionFactory to getSecurityPoolFactoryByteCode instead of contractAddresses.dualCapBatchAuctionFactory. These lines must be removed or updated to use the new dualCapBatchAuctionFactory to avoid runtime errors and incorrect deployments.

Removal of exported public API without clear replacement

📍 solidity/ts/testsuite/simulator/utils/contracts/peripherals.ts:125

The diff removes the export of ReportMeta interface and getTokenId utility function. While these may no longer be needed directly by consumers, they were previously part of the module's public API. Their removal could break downstream code that relied on these types or functions. In a stable utility module, especially one used across a test suite, removing public symbols without a deprecation period or documented migration path can cause unexpected failures and maintenance friction.

Broad module responsibility

📍 solidity/ts/testsuite/simulator/utils/contracts/peripherals.ts:1

The peripherals.ts module aggregates utilities for numerous distinct contract types (auction, oracle, token, market, WETH). This breadth of responsibility increases coupling between unrelated domains and violates the Single Responsibility Principle. Changes to any one peripheral (e.g., auction interface changes) occur in the same file as others, raising the risk of accidental modifications and making the module harder to understand and maintain. Ideally, peripheral interactions should be separated by domain or functional area.

deployZoltarTransaction removed from exports

📍 solidity/ts/testsuite/simulator/utils/contracts/zoltar.ts:20

The function deployZoltarTransaction was previously exported but is now defined as a private constant. This breaks backward compatibility for any code that imports and uses this function to obtain the deployment transaction for the Zoltar contract. A stable public API should retain such utility functions or provide a deprecation path.

splitRep parameter type changed to number[]

📍 solidity/ts/testsuite/simulator/utils/contracts/zoltar.ts:68

The outcomeIndexes parameter for splitRep changed from bigint[] to number[]. Blockchain indices can exceed the safe integer limit of JavaScript numbers, risking precision loss. This also breaks type compatibility for existing callers that pass bigint[]. Using bigint[] for blockchain-related indices ensures precision and consistency.

forkerClaimRep parameter type changed to number[]

📍 solidity/ts/testsuite/simulator/utils/contracts/zoltar.ts:85

The outcomeIndexes parameter for forkerClaimRep changed from bigint[] to number[]. Similar to splitRep, this may cause precision issues for large values and breaks type compatibility. For blockchain data, bigint[] should be used to avoid overflow and maintain interface consistency.

deployChild and getOutcomeName functions removed from exports

📍 solidity/ts/testsuite/simulator/utils/contracts/zoltar.ts

The functions deployChild and getOutcomeName were previously exported but are completely removed in the current version. This breaks backward compatibility for any code that relies on these functions for deploying child universes or retrieving outcome names. Such removals should be accompanied by deprecation warnings or migration guides to avoid runtime failures in dependent modules.

Overly complex and highly coupled test setup function

📍 solidity/ts/testsuite/simulator/utils/utilities.ts:68

The setupTestAccounts function (lines 68-120) violates single responsibility principle by combining impersonation, balance minting, contract deployment via state overrides, and storage layout manipulation. It is tightly coupled to specific contract implementations (ReputationToken storage slot 5, WETH9 address/bytecode, ProxyDeployer bytecode) via hard-coded values. This creates brittle tests that break on any contract change and mixes high-level test setup with low-level storage details.

High coupling to concrete test provider type AnvilWindowEthereum

📍 solidity/ts/testsuite/simulator/utils/viem.ts:9

The functions createReadClient and createWriteClient now accept AnvilWindowEthereum in their parameter union, directly coupling this utility module to a specific test provider implementation. This violates the Dependency Inversion Principle and the abstraction principle; the module should depend on abstractions (EIP1193Provider) to remain flexible and testable with different Ethereum providers. The need to add AnvilWindowEthereum indicates it does not properly conform to the EIP1193Provider interface, suggesting either a missing type definition in the Anvil provider or an inappropriate reliance on implementation details. This coupling will require modifications to this ubiquitous utility whenever a new provider type is introduced, hindering extensibility and maintainability.

Removal of runtime-specific type definitions breaks implicit dependency on Node.js/Bun

📍 solidity/tsconfig.json:19

The tsconfig.json no longer includes 'node' and 'bun-types' in the 'types' compiler option. This change removes global type definitions for Node.js and Bun runtimes. If the TypeScript code in 'ts/**/*.ts' uses global APIs from these runtimes (e.g., 'process', 'Buffer', Bun-specific globals) without explicit type imports, it will lead to implicit 'any' types or compilation errors. This violates the architectural principle of explicit dependencies and can introduce hidden coupling to runtime environments, hindering maintainability and portability. The change assumes code has been updated to import types explicitly, but without that context, it risks breaking type safety across the codebase.

@github-actions
Copy link

🔒 Security Reviewer

8 issues found

Deprecated ESLint version in use

📍 package-lock.json

The package-lock.json includes ESLint version 8.57.0 which is explicitly marked as deprecated and no longer supported. Without security updates, this version may contain unpatched vulnerabilities that could be exploited during linting (e.g., via malicious rule configurations or crafted code). It is strongly recommended to upgrade to ESLint v9 or later.

Outdated and vulnerable 'glob' dependency

📍 package-lock.json

The package-lock.json contains glob version 7.2.3, which is deprecated due to 'widely publicized security vulnerabilities' that have been fixed in newer releases. Using this version exposes the project to risks such as path traversal, ReDoS, and other file-handling exploits when glob patterns are processed. It should be updated to a supported version (>=8.x) immediately.

Lock file misconfiguration: incorrect name and location

📍 solidity/bun.lock

The project includes a lock file named bun.lock inside the solidity/ subdirectory. Bun expects a lock file named bun.lockb at the project root. Bun will ignore this file, causing dependencies to be installed without version pinning. This exposes the project to supply chain attacks such as dependency confusion or malicious package updates.

Unauthorized security pool setting

📍 solidity/contracts/peripherals/PriceOracleManagerAndOperatorQueuer.sol:51

The setSecurityPool function (lines 51-54) allows any address to set the securityPool variable, which is used for critical operations like setting the REP/ETH price and executing queued operations. Since this function can only be called once, an attacker can front-run the first call to set securityPool to a malicious contract, leading to complete control over the contract's security mechanisms. This includes arbitrarily setting lastPrice (affecting oracle calculations) and executing malicious operations via the securityPool, compromising the system's integrity.

Insecure Ether transfer using transfer()

📍 solidity/contracts/peripherals/WETH9.sol:24

The withdraw function uses transfer() to send Ether. transfer() only forwards 2300 gas and will fail when sending to contracts with non-trivial fallback functions, causing legitimate withdrawals to revert and potentially locking user funds. This is a known issue and should be replaced with a call() pattern with proper success checking.

Approve race condition vulnerability

📍 solidity/contracts/peripherals/WETH9.sol:35

The approve function does not first set the allowance to zero before setting it to a new value. This allows a malicious spender to front-run the transaction and use the old allowance (which may be higher than intended) to transfer tokens before the allowance is updated to the new value. This is a well-known ERC20 race condition.

Missing non-negative validation in SmallIntParser.serialize

📍 solidity/ts/testsuite/simulator/types/wire-types.ts:22

The SmallIntParser.serialize function does not check for negative values, unlike other similar parsers (BigIntParser, AddressParser, Bytes32Parser, etc.). This inconsistency allows negative bigint values to be serialized to hex strings (e.g., '-0x1'), which are invalid for unsigned Ethereum integer fields. Such invalid data could cause parsing errors downstream, transaction rejection, or unexpected behavior if used in security-critical logic.

contractExists function always returns true for valid addresses

📍 solidity/ts/testsuite/simulator/utils/utilities.ts:131

The contractExists function checks if client.getCode returns undefined, but getCode always returns a string (e.g., '0x' for EOAs). Therefore, it incorrectly returns true for externally owned accounts and any address without contract code. This can lead to misidentification of contract addresses and potential logic errors in the simulator.

@github-actions
Copy link

🧠 Wise Man

13 issues found

Deprecated 'glob' package with known security vulnerabilities

📍 package-lock.json

The package 'glob' version 7.2.3 is deprecated and contains widely publicized security vulnerabilities. It is strongly recommended to upgrade to a newer version (e.g., the latest major version) to mitigate security risks. This package is present in the dependency tree as a dev dependency.

Deprecated 'inflight' package with memory leak

📍 package-lock.json

The package 'inflight' version 1.0.6 is deprecated because it leaks memory. It is not supported and should be replaced. The deprecation message recommends using 'lru-cache' as an alternative. This package is present in the dependency tree as a dev dependency.

Unsupported 'eslint' version 8.57.0

📍 package-lock.json

The installed version of 'eslint' (8.57.0) is no longer supported. Using unsupported versions means missing out on security patches and new features. Please upgrade to a supported version (check the ESLint website for the latest recommended version).

Unsupported 'rimraf' version 3.0.2

📍 package-lock.json

The version of 'rimraf' (3.0.2) is no longer supported. Versions prior to v4 are deprecated. Upgrade to rimraf v4 or later to ensure ongoing compatibility and support.

Deprecated '@humanwhocodes/config-array' package

📍 package-lock.json

The package '@humanwhocodes/config-array' (version 0.11.14) is deprecated in favor of '@eslint/config-array'. This package will no longer receive updates. Update your dependencies to use the replacement package to avoid future breakage.

Deprecated '@humanwhocodes/object-schema' package

📍 package-lock.json

The package '@humanwhocodes/object-schema' (version 2.0.3) is deprecated in favor of '@eslint/object-schema'. Replace it with the maintained alternative to ensure continued support.

Inconsistent ABI field usage between ERC standards

📍 solidity/ts/abi/abis.ts:137

The ERC1155 ABI definition uses only the modern stateMutability field for functions, adhering to current Solidity ABI standards. In contrast, the ERC20 ABI includes both legacy constant and payable fields alongside stateMutability, which is redundant and inconsistent. This inconsistency reduces maintainability, increases cognitive load, and deviates from best practices. Standardize all ABI definitions to rely solely on stateMutability for state mutability, removing constant and payable fields.

Inconsistent numeric types for outcome indexes

📍 solidity/ts/tests/zoltar.test.ts:73

The test defines outcomeIndexes and splitOutcomeIndexes as number arrays, while the codebase consistently uses bigint for chain-related values (e.g., genesisUniverse = 0n, FORKER_DEPOSIT_FRACTION = 20n). This type mismatch can cause runtime errors when passing these arrays to functions that expect bigint (e.g., getChildUniverseId, forkerClaimRep, splitRep). To maintain type safety and readability, use bigint literals (0n, 1n, 2n, 3n) to align with the rest of the codebase.

Dead code in ANVIL_RPC fallback chain

📍 solidity/ts/testsuite/simulator/AnvilWindowEthereum.ts:39

The fallback chain for ANVIL_RPC uses two '||' operators, making the second fallback (to 'http://localhost:8545') unreachable because the first fallback ('http://host.docker.internal:8545') is always truthy. This is a logical error and the code will never use 'http://localhost:8545' as a default.

Dead code: null check on non-nullable type

📍 solidity/ts/testsuite/simulator/AnvilWindowEthereum.ts:184

In getTime, the variable block is of type GetBlockReturn (a union of two object types) and cannot be null. The check if (block === null) is therefore always false and dead code.

Incorrect contract reference in ensureInfraDeployed after refactoring

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:129

The ensureInfraDeployed function still references auctionFactory (both as a property of existence/contractAddresses and in the call to getSecurityPoolFactoryByteCode) even though the infrastructure was refactored to use dualCapBatchAuctionFactory. Additionally, the deployment of AuctionFactory is erroneously attempted using an unimported artifact. This will cause runtime ReferenceErrors and incorrect deployment behavior.

Missing import for DualCapBatchAuction contract artifact

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:7

The getSecurityPoolAddresses function uses peripherals_DualCapBatchAuction_DualCapBatchAuction, but this artifact is not imported. This will cause a ReferenceError when the truthAuction deployment code path is executed (i.e., when parent != 0).

Missing commas between type members causing syntax errors

📍 solidity/ts/types/index.d.ts:3

TypeScript interfaces and object type literals require commas (or semicolons) to separate members. The provided declaration file omits these separators in many places, resulting in a file that fails to compile. The diff exacerbated this by removing the trailing comma from the 'settings' property. The following lines need a trailing comma (or semicolon) added to restore validity. Note: for multi-line property types, the comma should be added after the closing brace if the property is not the last one.

@github-actions
Copy link

🐛 Bug Hunter

19 issues found

Incorrect module export syntax for ESLint plugin

📍 eslint-plugin-local/index.js:1

The file uses ES6 'export default' syntax but includes a CommonJS 'require' call. ESLint plugins typically expect a CommonJS module that exports a 'rules' object via 'module.exports'. This code will fail to load in standard Node.js environments without transpilation, causing a ReferenceError (in ES6 modules) or syntax error (in CommonJS), breaking plugin functionality.

Invalid package.json structure: missing comma in exports field

📍 eslint-plugin-local/package.json:5

The 'exports' field value is not properly terminated with a closing quote and brace for the JSON object. The provided snippet shows line 5 as '"exports": "./index.js"' without a closing quote for the value and without a closing brace for the object. This makes the entire package.json invalid JSON, which will cause 'npm install' or any tooling that reads package.json to fail with a JSON parse error.

Rule fails to enforce single-line requirement for multi-statement switch cases without braces

📍 eslint-rules/single-line-switch-case.js:16

The rule returns early if the switch case consequent does not contain exactly one statement (line 16). This causes it to miss violations where a switch case without braces has multiple statements spanning multiple lines, directly contradicting the rule's message that such cases must stay on one line. For example, a case like case 1: statement1; statement2; on separate lines would not be flagged, even though it violates the intended behavior.

Incorrect condition for requesting new price leads to revert when pending report exists

📍 solidity/contracts/peripherals/PriceOracleManagerAndOperatorQueuer.sol:135

The function requestPriceIfNeededAndQueueOperation checks else if (queuedPendingOperationId == 0) to decide whether to request a new price when the current price is invalid. This condition fails to account for a pending price request that was initiated by a direct call to requestPrice, which sets pendingReportId but leaves queuedPendingOperationId as zero. In such a scenario, the function incorrectly attempts to request a new price, causing a revert in requestPrice due to pendingReportId != 0. As a result, the operation fails to be queued and the transaction reverts, even though there is already a pending price request that will eventually make the price valid. The condition should also check that pendingReportId == 0 to ensure there is no pending report at all.

getAllFiles security check produces false positives when contracts directory is a symlink

📍 solidity/ts/compile.ts:163

The function getAllFiles performs a security check to ensure that each file is within the allowed base directory. For non-symlink files, targetPath is set to filePath (which may contain symlink names). When the top-level 'contracts' directory itself is a symlink, filePath contains the symlink name rather than the canonical path. The subsequent path.relative(baseDir, targetPath) resolves targetPath to an absolute path based on the current working directory, which differs from the real location under baseDir, causing the relative path to start with '..' and incorrectly throwing a 'Path traversal detected' error. This prevents legitimate builds from proceeding. All entries should have their realpath resolved before the check to avoid false positives.

Filename sanitization for contract artifact names produces invalid TypeScript identifiers and may cause duplicate names

📍 solidity/ts/compile.ts:197

In copySolidityContractArtifact, the filename part of the contract name is sanitized by removing dashes entirely and replacing slashes with underscores, but it does not handle other characters that are invalid in TypeScript identifiers (e.g., spaces, punctuation) nor ensures the name starts with a letter or underscore. This can generate TypeScript syntax errors if the resulting identifier contains spaces or starts with a digit. Additionally, removing dashes entirely can cause collisions between distinct filenames (e.g., my-contract.sol and mycontract.sol both become mycontract), leading to 'duplicated contract name!' errors. The sanitization should replace all non-identifier characters with underscores and ensure the result starts with a valid character.

Incorrect clearing calculation logic in computeExpectedClearing test helper

📍 solidity/ts/tests/auction/auction.test.ts:658

The function computeExpectedClearing is used to validate the contract's clearing behavior. It has multiple flaws: (1) It only checks newAccumulatedEth > maxEthAtThisTick for the REP cap, missing the exact hit case (>=) and potentially continuing after the REP cap is reached. (2) When the REP cap would be exceeded, it fills up to maxEthAtThisTick without considering that the ETH cap (ethRaiseCap) might be lower, which could result in accumulatedEth exceeding the allowed ETH cap. (3) It divides by price without guarding against price === 0n, which would throw a division-by-zero error for very negative ticks. These issues produce incorrect expected clearing values, may cause false test outcomes, and can lead to runtime crashes.

Missing commas in type definition for securityPoolAddresses

📍 solidity/ts/tests/peripherals.test.ts:28

The object type securityPoolAddresses is missing commas between its properties. TypeScript requires commas to separate type members. This will cause a compilation error and prevent the code from running.

Outcome indexes incorrectly typed as number instead of bigint

📍 solidity/ts/tests/zoltar.test.ts:73

The arrays 'outcomeIndexes' and 'splitOutcomeIndexes' contain numeric literals (e.g., 0, 1, 2) but are passed to functions like getChildUniverseId and getRepTokenAddress that expect bigint parameters. In JavaScript, arithmetic or operations mixing bigint and number throw a TypeError. This will cause runtime failures during test execution when these indexes are used to derive child universe IDs.

TimestampParser.parse crashes on empty hex string '0x'

📍 solidity/ts/testsuite/simulator/types/wire-types.ts:108

The parse function calls BigInt(value) directly after regex validation. The regex allows '0x' (zero hex digits), which causes BigInt to throw a SyntaxError, breaking the expected return pattern and leading to an unhandled exception for a valid input according to the regex.

BigIntParser.serialize lacks upper bound check for 256-bit limit

📍 solidity/ts/testsuite/simulator/types/wire-types.ts:9

The serialize method does not validate that the bigint value is less than 2^256. The parser only accepts up to 64 hex digits (maximum value 2^256-1). Serializing a value >= 2^256 produces a hex string longer than 64 digits, violating the wire format and causing round-trip failures.

TimestampParser.serialize does not validate Date validity

📍 solidity/ts/testsuite/simulator/types/wire-types.ts:112

The serialize method only checks that the input is an instance of Date, but does not verify that the date is valid (i.e., not NaN). Passing an invalid Date results in '0xNaN' as output, which is not a valid hex timestamp and will fail parsing later.

areEqualArrays does not handle NaN values correctly

📍 solidity/ts/testsuite/simulator/utils/array-utils.ts:4

The function uses strict inequality (!==) for element comparison, which returns true for NaN !== NaN, causing arrays with NaN at the same positions to be incorrectly reported as unequal. This is a functional error in equality checks for arrays containing NaN.

TEST_ADDRESSES element 4 breaks established address pattern

📍 solidity/ts/testsuite/simulator/utils/constants.ts:7

The fourth address in TEST_ADDRESSES uses a prefix of 12 zeros (0x000000000000...) instead of the consistent prefix of 11 zeros followed by a '1' (0x000000000001...) used by all other addresses. This alters the numeric value of the address and likely results from a typo during refactoring. The constant defines specific test accounts; the incorrect value will point to a different address than originally intended, causing tests that rely on this address to fail or interact with the wrong entity.

Stale reference to removed AuctionFactory in ensureInfraDeployed

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:129

The code at line 129 checks for and attempts to deploy a contract 'peripherals_factories_AuctionFactory_AuctionFactory' which is no longer imported and not part of the infrastructure. This will cause a ReferenceError when ensureInfraDeployed is called. The AuctionFactory has been replaced by DualCapBatchAuctionFactory and this line should be removed.

Incorrect property 'auctionFactory' used in SecurityPoolFactory deployment

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:131

In ensureInfraDeployed at line 131, the call to getSecurityPoolFactoryByteCode passes 'contractAddresses.auctionFactory' as an argument. However, getInfraContractAddresses returns 'dualCapBatchAuctionFactory', not 'auctionFactory'. This results in 'undefined' being passed, leading to invalid deployment data and likely a runtime error.

Missing import for DualCapBatchAuction contract

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:7

The getSecurityPoolAddresses function uses 'peripherals_DualCapBatchAuction_DualCapBatchAuction' (lines 222-223) but this symbol is not imported. This will cause a ReferenceError when getSecurityPoolAddresses is called with a non-zero parent. The import statement on line 7 must be updated to include this contract artifact.

getOpenOracleReportMeta returns bigints for number fields

📍 solidity/ts/testsuite/simulator/utils/contracts/peripherals.ts:156

The function getOpenOracleReportMeta returns values for fields that are typed as number (settlementTime, feePercentage, protocolFee, multiplier, disputeDelay) but directly returns the raw values from the contract read, which are bigints. This causes a type mismatch and will lead to runtime errors when these values are used as numbers (e.g., arithmetic operations or type checks). The getOpenOracleExtraData function in the same file correctly converts similar fields using Number(). The getOpenOracleReportMeta should apply the same conversions to ensure the returned object matches the ReportMeta interface.

Precision loss when converting deposit indexes to Number

📍 solidity/ts/testsuite/simulator/utils/contracts/securityPoolForker.ts:124

In migrateFromEscalationGame, the depositIndexes array (typed as bigint[]) is converted to a Number array via depositIndexes.map(x => Number(x)). This conversion loses precision for bigint values greater than Number.MAX_SAFE_INTEGER (2^53 - 1). Since deposit indexes may be large uint256 values in practice, this can result in incorrect indexes being passed to the smart contract, leading to wrong migration outcomes or data corruption.

@github-actions
Copy link

🔧 Expensive Linter

16 issues found

Inconsistent spacing in low-level call with value modifier

📍 solidity/contracts/peripherals/PriceOracleManagerAndOperatorQueuer.sol:99

The added refund blocks use call{ value: ... } with a space after the opening brace and before the closing brace. However, the existing call openOracle.createReportInstance{value: ethCost} (line 94) does not include such spaces. To maintain consistency across the file, all calls with the value modifier should follow the same formatting without spaces inside the braces, i.e., call{value: ...}.

Inconsistent function signature formatting

📍 solidity/contracts/peripherals/WETH9.sol:45

The function transferFrom has its signature split across multiple lines with public and returns (bool) on separate lines and the opening brace on a new line (lines 45-48), while all other functions in the file (e.g., deposit, withdraw, approve, transfer) have the entire signature on one line with the opening brace on the same line. This breaks consistency in code style within the same contract.

Leftover debugger statement in error handler

📍 solidity/ts/compile.ts:282

The catch block for compileContracts contains a 'debugger' statement which should be removed before deployment to prevent unintended pauses in production environments.

Overly long function signature for computeExpectedClearing

📍 solidity/ts/tests/auction/auction.test.ts

The function computeExpectedClearing was changed from a well-formatted multi-line signature to a single line exceeding 150 characters. This violates typical line length conventions and significantly reduces readability.

Import statements missing file extensions

📍 solidity/ts/tests/escalationGame.test.ts:2

Local module imports omit the .js file extension, while the existing codebase consistently includes .js extensions for such imports. This inconsistency may lead to module resolution errors in Node.js ES module environments and deviates from the repository's established style.

Missing commas in TypeScript type literal for securityPoolAddresses

📍 solidity/ts/tests/peripherals.test.ts:28

The type definition for securityPoolAddresses is missing commas between its properties, which is a syntax error in TypeScript and will cause the file to fail compilation.

Unnecessary spaces in template literal placeholders

📍 solidity/ts/tests/priceOracleSecurity.test.ts:59

Template literal placeholders contain spaces inside the curly braces (e.g., ${ ethCost } instead of ${ethCost}), which violates standard JavaScript/TypeScript style conventions that omit such spaces for consistency and readability.

Inconsistent string quotation marks

📍 solidity/ts/tests/zoltar.test.ts:70

The file mixes single (') and double (") quotes for ordinary string literals. The established codebase style uses single quotes for strings (as seen in the majority of messages and previous versions). The three error messages on lines 70, 75, and 97 use double quotes, creating inconsistency. All plain string literals should use single quotes unless template literals are required.

Inconsistent import ordering

📍 solidity/ts/testsuite/simulator/AnvilWindowEthereum.ts:1

Import statements are not grouped by type (third-party vs local) and not sorted alphabetically within groups. The third-party import 'funtypes' is placed between local imports, which deviates from common practices where third-party imports are grouped together and typically placed before local imports, with alphabetical ordering within each group.

Inconsistent error handling in OptionalBytesParser.serialize

📍 solidity/ts/testsuite/simulator/types/wire-types.ts:122

The OptionalBytesParser.serialize method includes a runtime check for BytesParser.serialize being undefined and throws an exception. This deviates from the consistent pattern used by all other parser serialize methods, which always return a result object of the form { success: boolean, value: ... } and never perform defensive undefined checks. Additionally, BytesParser.serialize is guaranteed to be defined at that point, making the check unnecessary.

Overly long line for TEST_ADDRESSES constant

📍 solidity/ts/testsuite/simulator/utils/constants.ts:7

The TEST_ADDRESSES array is written on a single extremely long line, making it hard to read and violating typical line length conventions. The original file formatted this array across multiple lines; the PR change collapses it into one line, which is a clear style deviation.

Inconsistent naming for dual cap batch auction factory

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:129

In the ensureInfraDeployed function, the variable 'auctionFactory' is used, but the standardized name across the codebase is 'dualCapBatchAuctionFactory'. This inconsistency appears in existence checks and function arguments, leading to potential bugs.

Missing import for DualCapBatchAuction contract

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:7

The contract 'peripherals_DualCapBatchAuction_DualCapBatchAuction' is used in getSecurityPoolAddresses but is not imported, causing a reference error at runtime.

Local module imports missing .js file extension

📍 solidity/ts/testsuite/simulator/utils/contracts/deployments.ts:1

The updated file imports local modules without the .js file extension (e.g., './deployPeripherals', '../bigint', '../../../../types/contractArtifact'), while the original codebase consistently included .js extensions for such imports. This may cause module resolution issues in environments that require explicit extensions or deviate from the project's established style.

participateAuction function missing return statement

📍 solidity/ts/testsuite/simulator/utils/contracts/peripherals.ts:181

The participateAuction function does not return the result of client.writeContract, causing it to return undefined instead of the expected transaction result. This is a regression from the previous version where a return statement was present, and it likely breaks functionality that depends on the return value.

Trailing comma in include array

📍 tsconfig.json:32

The include array in tsconfig.json has a trailing comma after its last element ("solidity/ts/**/*.mts"), which is inconsistent with the style used in the lib array that omits trailing commas. To maintain consistency, the last element should not have a trailing comma.

Copy link
Member

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

Would like to discuss the necessity of all the transient dependencies this PR brings in before proceeding.

Comment on lines +1 to +6
FROM ghcr.io/foundry-rs/foundry:v1.5.1
RUN useradd -m -u 1000 appuser
USER appuser
EXPOSE 8545
ENTRYPOINT ["anvil"]
CMD ["--host", "0.0.0.0", "--port", "8545", "--block-base-fee-per-gas", "0", "--gas-price", "0", "--no-priority-fee"]
Copy link
Member

Choose a reason for hiding this comment

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

This file is unnecessary. You can instead just do:

docker container run --rm -it ghcr.io/foundry-rs/foundry:v1.5.1 -- anvil --host 0.0.0.0 --port 8545 --block-base-fee-per-gas 0 --gas-price 0 --no-priority-fee

You could put that in a shell script if you want to turn it into something simpler.

Comment on lines +12 to +17
"eslint": "8.57.0",
"eslint-plugin-unused-imports": "4.4.1",
"funtypes": "5.1.1",
"globals": "15.9.0",
"knip": "5.86.0",
"prettier": "3.8.1",
Copy link
Member

Choose a reason for hiding this comment

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

While I appreciate the value of linters, formatters, and static analyzers in the AI era, are there not a set of tools we could use that don't bring in >100 transitive dependencies?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants