Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
MicahZoltu
left a comment
There was a problem hiding this comment.
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 |
🛡️ Defender Against the Dark Arts7 issues found Addition of deprecated 'glob' package with known security vulnerabilitiesThe 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 ( Hardcoded zero salt enables address prediction attacks📍 solidity/ts/testsuite/simulator/utils/contracts/deployments.ts:14 The function uses a hardcoded salt of Single-point failure through centralized factory reference📍 solidity/ts/testsuite/simulator/utils/contracts/deployments.ts:13 The deployment address is derived from 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. |
🏛️ Architect17 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 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 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 ( Incomplete refactoring: leftover references to removed auctionFactory📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:129 After removing 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 splitRep parameter type changed to number[]📍 solidity/ts/testsuite/simulator/utils/contracts/zoltar.ts:68 The forkerClaimRep parameter type changed to number[]📍 solidity/ts/testsuite/simulator/utils/contracts/zoltar.ts:85 The deployChild and getOutcomeName functions removed from exports📍 solidity/ts/testsuite/simulator/utils/contracts/zoltar.ts The functions Overly complex and highly coupled test setup function📍 solidity/ts/testsuite/simulator/utils/utilities.ts:68 The 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/BunThe 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. |
🔒 Security Reviewer8 issues found Deprecated ESLint version in useThe 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' dependencyThe 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 locationThe project includes a lock file named 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. |
🧠 Wise Man13 issues found Deprecated 'glob' package with known security vulnerabilitiesThe 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 leakThe 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.0The 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.2The 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' packageThe 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' packageThe 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 standardsThe ERC1155 ABI definition uses only the modern 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 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. |
🐛 Bug Hunter19 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 Incorrect condition for requesting new price leads to revert when pending report exists📍 solidity/contracts/peripherals/PriceOracleManagerAndOperatorQueuer.sol:135 The function getAllFiles security check produces false positives when contracts directory is a symlinkThe 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 namesIn 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., Incorrect clearing calculation logic in computeExpectedClearing test helper📍 solidity/ts/tests/auction/auction.test.ts:658 The function Missing commas in type definition for securityPoolAddresses📍 solidity/ts/tests/peripherals.test.ts:28 The object type 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 |
🔧 Expensive Linter16 issues found Inconsistent spacing in low-level call with value modifier📍 solidity/contracts/peripherals/PriceOracleManagerAndOperatorQueuer.sol:99 The added refund blocks use Inconsistent function signature formatting📍 solidity/contracts/peripherals/WETH9.sol:45 The function Leftover debugger statement in error handlerThe 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 Unnecessary spaces in template literal placeholders📍 solidity/ts/tests/priceOracleSecurity.test.ts:59 Template literal placeholders contain spaces inside the curly braces (e.g., 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 arrayThe 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. |
MicahZoltu
left a comment
There was a problem hiding this comment.
Would like to discuss the necessity of all the transient dependencies this PR brings in before proceeding.
| 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"] |
There was a problem hiding this comment.
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.
| "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", |
There was a problem hiding this comment.
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?
add Anvil