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

1450 thor client fix circular dependencies #1514

Merged
merged 36 commits into from
Nov 18, 2024

Conversation

lucanicoladebiasi
Copy link
Collaborator

Description

The code refactors the ThorClient to avoid circular dependencies among the modules composing the Thor client.

Fixes #1450

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • yarn test:examples
  • yarn test:solo
  • yarn test:unit

Test Configuration:

  • Node.js Version: v23.1.0
  • Yarn Version: 1.22.22

Checklist:

@lucanicoladebiasi lucanicoladebiasi self-assigned this Nov 13, 2024
Copy link

github-actions bot commented Nov 13, 2024

Test Coverage

Summary

Lines Statements Branches Functions
Coverage: 99%
98.99% (4345/4389) 97.53% (1387/1422) 98.9% (901/911)
Title Tests Skipped Failures Errors Time
core 827 0 💤 0 ❌ 0 🔥 2m 19s ⏱️
network 719 0 💤 0 ❌ 0 🔥 4m 59s ⏱️
errors 42 0 💤 0 ❌ 0 🔥 16.866s ⏱️
logging 3 0 💤 0 ❌ 0 🔥 18.588s ⏱️
hardhat-plugin 19 0 💤 0 ❌ 0 🔥 1m 0s ⏱️
aws-kms-adapter 23 0 💤 0 ❌ 0 🔥 1m 8s ⏱️
ethers-adapter 5 0 💤 0 ❌ 0 🔥 1m 11s ⏱️
rpc-proxy 37 0 💤 0 ❌ 0 🔥 1m 2s ⏱️

@freemanzMrojo
Copy link
Member

Hi @lucanicoladebiasi , I think we should modify the script at package.json that counts the number of circular dependencies to validate that indeed they are going down (as of now it counts up to 11).

The script is check:circular-dependencies at network's package.json 👍

@lucanicoladebiasi
Copy link
Collaborator Author

Hi @lucanicoladebiasi , I think we should modify the script at package.json that counts the number of circular dependencies to validate that indeed they are going down (as of now it counts up to 11).

The script is check:circular-dependencies at network's package.json 👍

I modified the script as

 "check:circular-dependencies": "npx madge --json --circular --extensions ts src | jq '. | length' | awk '{if($1 > 0) exit 1}'"

and it doesn't complain anymore.

@freemanzMrojo
Copy link
Member

Hi @lucanicoladebiasi , I think we should modify the script at package.json that counts the number of circular dependencies to validate that indeed they are going down (as of now it counts up to 11).
The script is check:circular-dependencies at network's package.json 👍

I modified the script as

 "check:circular-dependencies": "npx madge --json --circular --extensions ts src | jq '. | length' | awk '{if($1 > 0) exit 1}'"

and it doesn't complain anymore.

Nice! If we have 0 we could just run npx madge --circular --extensions ts src without the rest, like we do in package.jsonat core 👍

@lucanicoladebiasi lucanicoladebiasi marked this pull request as ready for review November 15, 2024 13:05
@lucanicoladebiasi lucanicoladebiasi requested a review from a team as a code owner November 15, 2024 13:05
Copy link
Member

@freemanzMrojo freemanzMrojo left a comment

Choose a reason for hiding this comment

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

LGTM, now looks much nicer, thanks!

Left some minor suggestions, regarding the constructor we could do the same also for nodes-module and transactions-module since looks like all args are readonly 👍

packages/network/package.json Outdated Show resolved Hide resolved
packages/network/src/thor-client/blocks/blocks-module.ts Outdated Show resolved Hide resolved
@victhorbi victhorbi removed this from the 1.0.0-RC.3 milestone Nov 18, 2024
@fabiorigam fabiorigam merged commit b0007a3 into main Nov 18, 2024
16 checks passed
@fabiorigam fabiorigam deleted the 1450-thor-client-fix-circular-dependencies branch November 18, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor thor-client.ts and remove circular dependencies
4 participants