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

refactor: decouple Logger and Provider generics #599

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

Wodann
Copy link
Member

@Wodann Wodann commented Aug 14, 2024

This change decouples the logger's error type from the provider. This prevents the necessity of knowing the Logger's error type when implementing traits for the provider-related types (errors, data, interval miner, etc.)

@Wodann Wodann added the no changeset needed This PR doesn't require a changeset label Aug 14, 2024
@Wodann Wodann requested a review from agostbiro August 14, 2024 01:36
@Wodann Wodann self-assigned this Aug 14, 2024
Copy link

changeset-bot bot commented Aug 14, 2024

⚠️ No Changeset found

Latest commit: 544bfe1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Wodann Wodann temporarily deployed to github-action-benchmark August 14, 2024 01:36 — with GitHub Actions Inactive
Copy link
Member

@agostbiro agostbiro left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM overall.

As a note, I'm not that big on Box<dyn std::error::Error + Send + Sync>> in libraries due to type erasure, but here the only consumer is the JS interface and there is type erasure anyway due to the FFI, so I don't think it's worth spending time on having richer error type in the logger.

@Wodann
Copy link
Member Author

Wodann commented Aug 16, 2024

Thanks, LGTM overall.

As a note, I'm not that big on Box<dyn std::error::Error + Send + Sync>> in libraries due to type erasure, but here the only consumer is the JS interface and there is type erasure anyway due to the FFI, so I don't think it's worth spending time on having richer error type in the logger.

I agree with you that type erasure should be avoided.

With a refactor I did yesterday and an idea I had overnight, there is one alternative approach that I could try to retain the LoggerErrorT. It will be especially beneficial if in Hardhat v3 we allow richer error types to be returned instead of strings.

I'm happy to give that a try now or to create an issue for it. Up to you.

@agostbiro
Copy link
Member

I'm ok with it as is

@Wodann
Copy link
Member Author

Wodann commented Aug 16, 2024

I created a follow-up issue.

@Wodann Wodann merged commit f0ebfd0 into feat/multichain Aug 16, 2024
38 checks passed
@Wodann Wodann deleted the refactor/logger branch August 16, 2024 17:29
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no changeset needed This PR doesn't require a changeset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants