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

client: fix misc tsc issues #3349

Merged
merged 3 commits into from
Apr 16, 2024
Merged

client: fix misc tsc issues #3349

merged 3 commits into from
Apr 16, 2024

Conversation

gabrocheleau
Copy link
Contributor

This PR addresses misc TypeScript compile issues that were being flagged up by tsc.

Copy link

codecov bot commented Apr 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.25%. Comparing base (4be68d2) to head (2755afa).
Report is 5 commits behind head on master.

❗ Current head 2755afa differs from pull request most recent head a39e03e. Consider uploading reports for the commit a39e03e to get more accurate results

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block ?
blockchain ?
client ?
common ?
devp2p ?
evm ?
genesis ?
statemanager ?
trie ?
tx ?
util ?
vm ?
wallet 87.25% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member

Updated this via UI

@@ -280,7 +280,7 @@ describe('[AccountFetcher]', async () => {
})

it('should reject zero-element proof if elements still remain to right of requested range', async () => {
const config = new Config({ transports: [], accountCache: 10000, storageCache: 1000 })
const config = new Config({ accountCache: 10000, storageCache: 1000 })
Copy link
Member

Choose a reason for hiding this comment

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

Might be off with this, but I think setting the transports here to empty was done to not have e.g. the RLPx layer started on tests here (and other occurrences as well). So this change might have the side-effect to reintroduce this for the test runs which is not what we want. We should double check here.

(I generally find this a bit problematic to do an eventual functional change just for typing reasons. What was the complaint here? Is there maybe a solution possible to adjust the type or something? 🤔 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual TS issue in this case is the following:

Object literal may only specify known properties, and 'transports' does not exist in type 'ConfigOptions'.ts(2353)
(property) transports: never[]

It seems like the transports property was removed in this PR: #3069

Not sure why this should be kept?

Copy link
Contributor

Choose a reason for hiding this comment

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

Second what @gabrocheleau said. transports no longer exists as a config option since we only support RPLx and not libp2p.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM!

@acolytec3 acolytec3 merged commit babbe20 into master Apr 16, 2024
34 checks passed
@holgerd77 holgerd77 deleted the client/fix-tsc-issues branch April 16, 2024 13:19
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.

3 participants