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

Rearranged type param order so that the Network param is the last #587

Merged
merged 3 commits into from
Mar 30, 2024

Conversation

moricho
Copy link
Contributor

@moricho moricho commented Mar 27, 2024

Motivation

To default Network parameters to Ethereum in alloy-provider and alloy-contract, they need to be moved to the last of the type params: alloy-rs/alloy#356
According to those changes, we need to adjust the type param order in sol! as well.

from: alloy-rs/alloy#263

Solution

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@moricho moricho requested a review from DaniPopes as a code owner March 27, 2024 17:07
@moricho moricho changed the title Rearranged type parameters so that the Network parameter is the last Rearranged type param order so that the Network param is the last Mar 27, 2024
@moricho
Copy link
Contributor Author

moricho commented Mar 27, 2024

@onbjerg Thanks for your advice in alloy-rs/alloy#356 (comment). I've opened this PR to adjust the type param order in sol!

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

that makes sense,

does this introduce more issues with patched alloy-core deps on alloy?

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this currently clashes with alloy-provider, no?

https://github.com/alloy-rs/alloy/blob/c22b9ddd6461793f7bf05d5bb32a3c0fb2b8fe71/crates/provider/src/provider.rs#L198-L198

so this is a alloy-provider issue first

EDIT: I should read the PR more closely

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Thanks! I made the generated struct also default to N = Ethereum. Will merge once companion PR alloy-rs/alloy#356 is ready.

@moricho
Copy link
Contributor Author

moricho commented Mar 29, 2024

@mattsse

does this introduce more issues with patched alloy-core deps on alloy?

This PR will only affect alloy-contract and alloy-provider and it's already covered by the PR I created in alloy-rs/alloy.
(sol! is being used only for tests in the packages for now)

@moricho
Copy link
Contributor Author

moricho commented Mar 29, 2024

@DaniPopes Thanks! I've just fixed alloy-rs/alloy#356 and it's ready now. Once this PR is merged, I'll update alloy dependencies to use the latest alloy-core

@DaniPopes DaniPopes merged commit 2fcea76 into alloy-rs:main Mar 30, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants