[WIP] Full Code Review #24

iamnp · 2024-10-26T09:33:39Z

API-centered code review of the Depot project

…n tests

…rettier

Adding hardhat-keystore

Parse trace logs

… console). Retry in case of rate limit

feat: use cache only if it's enabled + refactor

Bumps [path-to-regexp](https://github.com/pillarjs/path-to-regexp) from 6.2.2 to 6.3.0. - [Release notes](https://github.com/pillarjs/path-to-regexp/releases) - [Changelog](https://github.com/pillarjs/path-to-regexp/blob/master/History.md) - [Commits](pillarjs/path-to-regexp@v6.2.2...v6.3.0) --- updated-dependencies: - dependency-name: path-to-regexp dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>

…to-regexp-6.3.0 build(deps): bump path-to-regexp from 6.2.2 to 6.3.0

Depot docs review

Feat/docs

src/common/bytes.ts

+function isValid(bytes: unknown): bytes is HexStr {
+  if (typeof bytes !== "string") return false;
+  const stripped = strip0x(bytes);
+  return stripped.length % 2 === 0 && /^[a-fA-f0-9]+$/.test(strip0x(bytes));


To fix the problem, we need to correct the regular expression to ensure it only matches valid hexadecimal characters. The best way to do this is to replace the overly permissive range A-f with the correct range A-F. This change will ensure that the regular expression only matches characters in the ranges 0-9, a-f, and A-F.

General fix: Replace the incorrect range A-f with the correct range A-F.

Detailed fix: Update the regular expression on line 68 in the file src/common/bytes.ts to use A-F instead of A-f.

Specific changes: Modify the regular expression in the isValid function to /^[a-fA-F0-9]+$/.

Requirements: No additional methods, imports, or definitions are needed to implement this change.

iamnp

TL;DR

Overall, the code is good, with several improvements of varying severity suggested, mostly aimed at improving the quality of API usage
Most of the decisions made by the project authors are reasonable and in line with the project's objectives
Two critical issues have been identified that require particular attention:
- OmnibusPlan has fields with deferred completion that may be ambiguous to use: link
- The omnibus test file structure is very spread out and contains many boilerplate code, giving a huge field for errors: link

Note

Severity ratings:
⚠️ - CRITICAL
🔴 - HIGH
🟡 - MEDIUM
👍 - GOOD IDEA

API Design

Requirements

APIs should be simple, concise, and easy to use. API should remain minimalist to reduce the amount of code the user must write. The framework should encapsulate complex logic to simplify API usage. Functions and objects should have descriptive names that clearly reflect their purpose and usage.

Review Points

Check that API functions and parameters are self-explanatory and require minimal documentation. Look for any extraneous methods or properties that could be removed or encapsulated within the framework. Look for areas where logic could be encapsulated, such as transaction handling or error management.

Findings

The project appears to be logically structured, with modular directories organised by functionality, including configurations, contracts, keystores, voting and trace handling, which helps with navigation and finding specific functionality. The naming conventions are generally well aligned with the expected functionality (e.g. providers, omnibuses, traces). Some directories, such as `omnibuses', contain broad functionality (e.g. tools, checks, blueprints) which might benefit from more granular sub-modules to clarify purpose at a glance. Moreover, enhancing function and parameter names, as suggested, will make the API more intuitive, leading to easier adoption and fewer errors during implementation. See code-related findings tagged with [API Design].

Error Prevention

Requirements

APIs should be bug-proof with no room for ambiguity, making it difficult for developers to misunderstand them. Design APIs to prevent misuse using intuitive naming conventions.

Review Points

Ensure that all API functions handle errors gracefully, with clear messages or error types for developers. Evaluate whether all exposed functions are logically grouped and that any complex functionality is encapsulated to simplify usage.

Findings

The codebase uses custom error classes, error handling and type-safe definitions. However, improvements could be made by extending the use of exhaustive checks for unsupported conditions. This would make the APIs more resilient to misuse and minimize potential edge cases that could lead to unhandled errors. See code-related findings tagged with [Error Prevention].

Code Quality

Requirements

Ensure robust TypeScript type definitions across the codebase to catch potential type errors at compile time. Each API function should include concise documentation outlining usage, parameters, and any edge cases.

Review Points

Check that all functions and variables are appropriately typed and that type inference is used wherever possible. Ensure function and module-level documentation is present and clear.

Findings

Overall, typing and type inference are used effectively within the project, with little room for improvement. The design seems to focus on encapsulating complex interactions. The presence of README files and examples of configuration files indicates some attention to usability. However, many files lack module and/or function level documentation, it may not be extremely useful to document everything, but focus on API documentation should be a priority. See code-related findings tagged with [Code Quality].

Testing

Requirements

The code should be rigorously tested, focusing on API behaviour under various edge cases. Where applicable, tests should mock Ethereum transactions to simulate and validate API behaviour.

Review Points

Ensure that tests cover a wide range of scenarios, including edge cases, expected behaviour and potential misuse. Verify that tests include mock transactions to ensure that the framework behaves predictably under simulated conditions.

Findings

It looks like mock data and utilities, including stubs and simulated transaction environments, are being used effectively. It is important to control test coverage and increase it from 60% to 80%+.

As a general suggestion, clear guidelines and conventions should be introduced for writing both omnibuses and, more importantly, tests.

Ability to run omnibus items one-by-one will decrease development time

iamnp · 2024-10-26T09:46:38Z

src/networks.ts

+
+export type ChainId = "1" | "5";
+export type ChainName = "eth" | "arb" | "opt";
+export type NetworkName = "mainnet" | "goerli";


[Code Quality] 🔴
Using the NetworkName type here for validation is good, but I suggest going further:
introduce the constants "mainnet" and "goerli" so that they can be referenced without having to type a string literal each time.

The same should be probably valid for ChainId and ChainName types.

iamnp · 2024-10-26T09:47:40Z

src/networks.ts

+
+const NETWORKS: Record<ChainName, Record<NetworkName, Network>> = {
+  eth: {
+    mainnet: new Network("mainnet", 1),


[Code Quality] 🔴
String literals should be replaced by constants here and everywhere else.

iamnp · 2024-10-26T09:48:51Z

omnibuses/_example_omnibus.ts

+import { call, event } from "../src/votes";
+
+export default omnibuses.create({
+  network: "mainnet",


[Code Quality] 🔴
The constant (to be introduced in src/networks.ts) should be used here instead of the string literal.

iamnp · 2024-10-26T09:58:35Z

omnibuses/_example_omnibus.ts

+
+export default omnibuses.create({
+  network: "mainnet",
+  // launchedOn: 12345678, // Launch block number should be set only if omnibus was successfully launched.


[API Design] ⚠️
The following fields:
launchedOn, voteId, executedOn, quorumReached should be set separately from the original omnibus plan being written and tested.

These fields should be separated into a different entity, as it is currently misleading as to how and when these params should be set, as they modify the original object being tested. Ideally, it should be possible to call separate callback-style functions such as omnibusStarted(voteId) etc. to give a clear timeline and explicit control over the omnibus object. These functions will modify the omnibus object internally.

iamnp · 2024-10-26T10:00:10Z

omnibuses/_example_omnibus.ts

+  // launchedOn: 12345678, // Launch block number should be set only if omnibus was successfully launched.
+  // voteId: 000, // Vote ID should be set only if omnibus is already started.
+  // executedOn: 12345678,  // Execution block number should be set only if vote is passed and omnibus was successfully executed.
+  quorumReached: false, // Should be set to true if quorum was reached during the vote.


[API Design] ⚠️
It should be considered to rethink the name and purpose of this field, as omnibuses are run until the quorum is reached, so there is no way that the final state could be quorumReached: false.

iamnp · 2024-10-26T11:21:13Z

src/omnibuses/omnibuses.ts

+  /**
+   * Contains the info about the omnibus launching - the number of the block where the omnibus was launched.
+   */
+  launchedOn?: number | undefined;


[API Design] 🔴
launchedOn and executedOn should be renamed to include context, e.g., launchedOnBlock, executedOnBlock. Correct terminology should be used, launch and execute are words with a similar meaning, probably enact would work better here?

iamnp · 2024-10-26T11:23:24Z

src/providers/providers.ts

+import { RpcProvider } from "./types";
+import { isHardhatEthersProvider, isJsonRpcProvider, UnsupportedProviderError } from "./utils";
+
+function provider(runner: ContractRunner): RpcProvider {


[Code Quality] 🟡
A validateRpcProvdier should be more suitable here to clearly indicate that the function is validating and returning a specific type of provider.

iamnp · 2024-10-26T11:42:00Z

src/omnibuses/blueprints/easy-track.ts

+  const { easyTrack, callsScript, voting } = contracts;
+  return {
+    title: title,
+    evmCall: call(easyTrack.removeEVMScriptFactory, [factory]),


[Error Prevention] 🟡
rename call?

iamnp · 2024-10-26T11:42:20Z

src/omnibuses/blueprints/easy-track.ts

+  contracts: Contracts<Lido>,
+  { factory, title }: RemovePaymentEvmScriptFactoryInput,
+): OmnibusItem {
+  const { easyTrack, callsScript, voting } = contracts;


[Error Prevention] 🟡
Do we need to validate input here?

Check factory, title against common mistakes (0 address, etc.)

iamnp · 2024-10-26T12:05:07Z

src/common/env.ts

+  }
+}
+
+function checkEnvVars() {


[Error Prevention] ⚠️
Is it enough to check only one var? I suggest checking vars when running tests too

fix: update WHALE, remove outdated item, fix readme link

Psirex and others added 30 commits November 29, 2023 13:45

Huge refactoring

39f029c

Rename imports in omnibus 2023_10_03

271a520

Move JsonBuilder in a standalone file. Add tests

57a47ae

Rework traces, contract-info-resolver. Add trace tests.

c4bb74b

Add the README stub

8f2afb5

Add simple install instruction to the README. Fix typechain first launch

4b41347

Rework hardhat-keystores package

fd82ee1

Update contract info resolver name

7e855ce

Rework omnibus item class. Test omnibus by item not by check type

da3caf5

feat: added tests for hardhat named-keystores-storage, silenced broke…

f62fb7d

…n tests

fix: remove unnecessary env warning, add fake env value to tests env

2ad83a2

fix: add typechain creation

d7e4c73

fix: add typechain creation

de35893

fix: remove step

a63b61c

fix: added readdir stub to tests

0b51101

fix: replaced docker action with actions check, fix code style with p…

4c1edce

…rettier

feat: added tests for named-keystore.ts, added test coverage to report

7efbe46

fix: import crypto

f0fbecc

feat: added tests for named-keystores.ts

ee747ad

fix: change prettier config following core repo

811b1df

fix: code style

9e8aaaa

fix: review comments

866ba82

fix: read keystores from FS only once on creation step

13d306f

fix: typechain error

0b935af

fix: use flag to read FS only once

2160c4b

Merge pull request #10 from lidofinance/feat/hardhat-keystore

6b224e8

Adding hardhat-keystore

feat: action UpdateStakingModule added

413470c

feat: action TransferAssetsInput added

729daa0

feat: refactoring, freeze hardhat version to 2.16.1

71ea945

fix: freeze node version in CI

d51a052

Ivan-Feofanov and others added 25 commits October 2, 2024 16:19

fix: typos and rewrite

1f74d03

fix: typo

2296d79

fix: rewrite

c33586b

feat: rename actions to blueprints, rewrite documentation

bcd76c0

fix: set correct image name, return omnibus test into global run

428b7db

fix: clean description

2823f8a

fix: return tracing contract

18b47ad

Merge pull request #19 from lidofinance/feat/parse-trace-logs

44bd073

Parse trace logs

feat: use cache nly if it's enabled + refactor

59d43ea

feat: don't throw an error if contract cannot be resolved (but log in…

f1e420a

… console). Retry in case of rate limit

fix: fixes and rebase to main

4ddf79e

fix: linter

7cc1bdc

fix: correct image name

b9114b8

fix: naming

7b98cfb

Review edits

c3c5c7d

Merge pull request #16 from lidofinance/feat/tracer-fixes

4d7997a

feat: use cache only if it's enabled + refactor

Add params descs

9422edf

Merge pull request #22 from lidofinance/dependabot/npm_and_yarn/path-…

5e929bd

…to-regexp-6.3.0 build(deps): bump path-to-regexp from 6.2.2 to 6.3.0

Remove unused param

09bdb40

pnpm style

3f13595

Add description of local node requirement

87564db

fix: style

77ee1aa

Merge pull request #21 from lidofinance/feat/docs-review.np

bc743b5

Depot docs review

Merge pull request #20 from lidofinance/feat/docs

49b2eca

Feat/docs

github-advanced-security bot found potential problems Oct 26, 2024

View reviewed changes

iamnp self-assigned this Oct 26, 2024

iamnp commented Oct 26, 2024

View reviewed changes

BATMAH69 and others added 2 commits November 19, 2024 20:50

fix: update WHALE, remove outdated item, fix readme link

966e8f4

Merge pull request #25 from BATMAH69/fix/update-whale

d8a4732

fix: update WHALE, remove outdated item, fix readme link

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Full Code Review #24

[WIP] Full Code Review #24

iamnp commented Oct 26, 2024 •

edited

Loading

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

iamnp left a comment •

edited

Loading

iamnp Oct 26, 2024

iamnp Oct 26, 2024

iamnp Oct 26, 2024

iamnp Oct 26, 2024

iamnp Oct 26, 2024

iamnp Oct 26, 2024 •

edited

Loading

iamnp Oct 26, 2024

iamnp Oct 26, 2024 •

edited

Loading

iamnp Oct 26, 2024

iamnp Nov 18, 2024

iamnp Oct 26, 2024

[WIP] Full Code Review #24

Are you sure you want to change the base?

[WIP] Full Code Review #24

Conversation

iamnp commented Oct 26, 2024 • edited Loading

iamnp left a comment • edited Loading

Choose a reason for hiding this comment

TL;DR

API Design

Requirements

Review Points

Findings

Error Prevention

Requirements

Review Points

Findings

Code Quality

Requirements

Review Points

Findings

Testing

Requirements

Review Points

Findings

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamnp Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamnp Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamnp commented Oct 26, 2024 •

edited

Loading

iamnp left a comment •

edited

Loading

iamnp Oct 26, 2024 •

edited

Loading

iamnp Oct 26, 2024 •

edited

Loading