Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce the use of path aliases in @dendreth/relay #365

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

EmilIvanichkovv
Copy link
Contributor

@EmilIvanichkovv EmilIvanichkovv commented Jul 8, 2024

This PR introduces the use of path aliases and sets up the project to support them.

While this change might seem like a simple refactor, it uncovered numerous issues with the overall configuration of the monorepo. Debugging and resolving these problems involved running our build scripts: yarn check:build and yarn build-plonky-2.

Here is a list of problems encountered and the fixes implemented:

  • refactor(relay): Use @ for paths within /relay

  • build(@dendreth/solidity): Install tsconfig-paths

  • config(@dendreth/solidity): Update hardhat.config.ts to use tsconfig-paths

    Hardhat does not natively support TypeScript path aliases out of the box.
    However, we can use tsconfig-paths package to achieve this.

    With this we fix errors like this:

    An unexpected error occurred:
    
    Error: Your application tried to access @, but it isn't declared in your dependencies;
    this makes the require call ambiguous and unsound.
    
    Required package: @ (via "@/constants/network_config.json")
    Required by: <path>/DendrETH/relay/utils/
    
  • build(check:build): Refactor script to run tsc with --build flag

    The --build option can be seen as a build orchestrator that find referenced
    projects, check if they are up-to-date, and build out-of-date projects in the
    correct order.

    The path aliases changes we want to introduce, forces the need of referenced
    projects.

  • config(tsconfig.json): Streamline the configuration to work with path aliases

    compilerOptions updates:

    • "composite": true
    • "rootDir": "./"
    • "declaration": true
    • "declarationMap": true

    Add references to the relay workspace
    Make sure all ts files are included

  • config(tsconfig.json): Make sure all json files are included

    This change resolves problems like this:

    error TS6307: File '<path>/DendrETH/beacon-light-client/plonky2/input_fetchers/balance_verification/abi/lido_accounting_oracle_abi.json' is not listed within the file list of project '<path>DendrETH/relay/tsconfig.json'. Projects must list all files or use an 'include' pattern.
    
    7 import accountingOracleAbi from '../../abi/lido_accounting_oracle_abi.json';
    
  • build(yarn): Unplug @lodestar/types@1.17.0

  • config(relay): Add @lodestar/types in paths.

    This change fixes following problems:

    ```
    relay/implementations/beacon-api.ts:543:9 - error TS2742: The inferred type of 'getBeaconBlock' cannot be named without a reference to '../../.yarn/unplugged/@lodestar-types-npm-1.17.0-1607a25762/node_modules/@lodestar/types/lib/u tils/executionAddress'. This is likely not portable. A type annotation is necessary.
    
    543   async getBeaconBlock(slot: bigint) {
                ~~~~~~~~~~~~~~
    
    relay/implementations/beacon-api.ts:574:9 - error TS2742: The inferred type of 'getBeaconState' cannot be named without a reference to '../../.yarn/unplugged/@lodestar-types-npm-1.17.0-1607a25762/node_modules/@lodestar/types/lib/utils/executionAddress'. This is likely not portable. A type annotation is necessary.
    
    574   async getBeaconState(slot: bigint) {
                ~~~~~~~~~~~~~~
    
    Found 2 errors.
    ```
    
  • config(tsconfig.json): Exclude dist dir

    This fixes problems like:

    error TS5055: Cannot write file <file-path> because it would overwrite input file.
    
  • build(build-plonky-2): Refactor script to use --build flag

  • config(@dendreth/balance-verification): Add project references

    The @dendreth/balance-verification workspace rely on files from @dendreth/relay.
    That is why we need to add this reference.

    With this change following proble was resolved:
    Running yarn build-plonky-2 ends in:

    ```
    error TS2688: Cannot find type definition file for 'node'.
      The file is in the program because:
        Entry point of type library 'node' specified in compilerOptions
    
    relay/implementations/beacon-api.ts:543:9 - error TS2742: The inferred type of 'getBeaconBlock' cannot be named without a reference to '../../.yarn/unplugged/@lodestar-types-npm-1.17.0-1607a25762/node_modules/@lodestar/types/lib/utils/executionAddress'. This is likely not portable. A type annotation is necessary.
    
    543   async getBeaconBlock(slot: bigint) {
                ~~~~~~~~~~~~~~
    
    relay/implementations/beacon-api.ts:574:9 - error TS2742: The inferred type of 'getBeaconState' cannot be named without a reference to '../../.yarn/unplugged/@lodestar-types-npm-1.17.0-1607a25762/node_modules/@lodestar/types/lib/utils/executionAddress'. This is likely not portable. A type annotation is necessary.
    
    574   async getBeaconState(slot: bigint) {
                ~~~~~~~~~~~~~~
    
    Found 3 errors in the same file, starting at: relay/implementations/beacon-api.ts:543
    ```
    
  • build(@dendreth/balance-verification): Add @types/node

  • refactor(relay): Remove unused imports

@EmilIvanichkovv EmilIvanichkovv force-pushed the fix_paths-emil2 branch 2 times, most recently from b375012 to ee234b4 Compare July 8, 2024 17:08
@EmilIvanichkovv EmilIvanichkovv changed the title Fix paths.. maybe Introduce the use of path aliases in @dendreth/relay Jul 8, 2024
@EmilIvanichkovv EmilIvanichkovv self-assigned this Jul 8, 2024
@EmilIvanichkovv EmilIvanichkovv added the enhancement New feature or request label Jul 8, 2024
relay/implementations/redis.ts Outdated Show resolved Hide resolved
relay/utils/orchestrator.ts Outdated Show resolved Hide resolved
relay/utils/orchestrator.ts Outdated Show resolved Hide resolved
smanilov
smanilov previously approved these changes Jul 9, 2024
Copy link

@smanilov smanilov left a comment

Choose a reason for hiding this comment

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

Impressive change! Glad to see path aliases working.

tsconfig.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@@ -3,7 +3,9 @@
"compilerOptions": {
"baseUrl": ".",
"paths": {
"@/*": ["./*"]
"@/*": ["./*"],
"@/lodestar-types": ["../.yarn/unplugged/@lodestar-types-npm-1.17.0-1607a25762/node_modules/@lodestar/types/lib/utils/executionAddress"]
Copy link

Choose a reason for hiding this comment

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

re: @lodestar-types-npm-1.17.0-1607a25762

Isn't it a problem that we're hardcoding a version here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are unplugging specific version:

    "@lodestar/types@1.17.0": {
      "unplugged": true
      }

Or do you refer to the hash after the version?

Copy link

Choose a reason for hiding this comment

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

No, I was referring to 1.17.0. Okay, I guess... So what happens when lodestar upgrades?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a valid question. I guess we have to re-unplug? And update the path here. Hmm..

tsconfig.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
kkirkov
kkirkov previously approved these changes Jul 9, 2024
Copy link
Contributor

@kkirkov kkirkov left a comment

Choose a reason for hiding this comment

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

Good work

kkirkov and others added 11 commits July 11, 2024 16:11
…ig-paths`

Hardhat does not natively support TypeScript path aliases out of the box.
However, we can use `tsconfig-paths` package to achieve this.

With this we fix errors like this:
```
An unexpected error occurred:

Error: Your application tried to access @, but it isn't declared in your dependencies;
this makes the require call ambiguous and unsound.

Required package: @ (via "@/constants/network_config.json")
Required by: <path>/DendrETH/relay/utils/
```
The `--build` option can be seen as a build orchestrator that find referenced
projects, check if they are up-to-date, and build out-of-date projects in the
correct order.

The path aliases changes we want to introduce, forces the need of referenced
projects.
… aliases

`compilerOptions` updates:
- "composite": true
- "rootDir": "./"
- "declaration": true
- "declarationMap": true

Add `references` to the `relay` workspace
Make sure all ts files are included
This change resolves problems like this:
```
error TS6307: File '<path>/DendrETH/beacon-light-client/plonky2/input_fetchers/balance_verification/abi/lido_accounting_oracle_abi.json' is not listed within the file list of project '<path>DendrETH/relay/tsconfig.json'. Projects must list all files or use an 'include' pattern.

7 import accountingOracleAbi from '../../abi/lido_accounting_oracle_abi.json';
```
This change fixes following problems:
```
relay/implementations/beacon-api.ts:543:9 - error TS2742: The inferred type of 'getBeaconBlock' cannot be named without a reference to '../../.yarn/unplugged/@lodestar-types-npm-1.17.0-1607a25762/node_modules/@lodestar/types/lib/utils/executionAddress'. This is likely not portable. A type annotation is necessary.

543   async getBeaconBlock(slot: bigint) {
            ~~~~~~~~~~~~~~

relay/implementations/beacon-api.ts:574:9 - error TS2742: The inferred type of 'getBeaconState' cannot be named without a reference to '../../.yarn/unplugged/@lodestar-types-npm-1.17.0-1607a25762/node_modules/@lodestar/types/lib/utils/executionAddress'. This is likely not portable. A type annotation is necessary.

574   async getBeaconState(slot: bigint) {
            ~~~~~~~~~~~~~~

Found 2 errors.
```
This fixes problems like:
```
error TS5055: Cannot write file <file-path> because it would overwrite input file.
```
The `@dendreth/balance-verification` workspace rely on files from `@dendreth/relay`.
That is why we need to add this reference.

With this change following proble was resolved:
Running `yarn build-plonky-2` ends in:
```
error TS2688: Cannot find type definition file for 'node'.
  The file is in the program because:
    Entry point of type library 'node' specified in compilerOptions

relay/implementations/beacon-api.ts:543:9 - error TS2742: The inferred type of 'getBeaconBlock' cannot be named without a reference to '../../.yarn/unplugged/@lodestar-types-npm-1.17.0-1607a25762/node_modules/@lodestar/types/lib/utils/executionAddress'. This is likely not portable. A type annotation is necessary.

543   async getBeaconBlock(slot: bigint) {
            ~~~~~~~~~~~~~~

relay/implementations/beacon-api.ts:574:9 - error TS2742: The inferred type of 'getBeaconState' cannot be named without a reference to '../../.yarn/unplugged/@lodestar-types-npm-1.17.0-1607a25762/node_modules/@lodestar/types/lib/utils/executionAddress'. This is likely not portable. A type annotation is necessary.

574   async getBeaconState(slot: bigint) {
            ~~~~~~~~~~~~~~

Found 3 errors in the same file, starting at: relay/implementations/beacon-api.ts:543
```
@EmilIvanichkovv EmilIvanichkovv force-pushed the fix_paths-emil2 branch 5 times, most recently from bbb2882 to 9db398d Compare July 11, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants