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

Remove the eip712 feature flag and have it enabled by default #2409

Merged
merged 8 commits into from
May 10, 2023

Conversation

dbelv
Copy link
Contributor

@dbelv dbelv commented May 9, 2023

Motivation

Closes #2408

Solution

Remove feature flag enabling by default.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@dbelv
Copy link
Contributor Author

dbelv commented May 9, 2023

Muscle memory had me doing cargo fmt which is unfortunately why large amount of touched files within the same commit vs the actual implementation of the issue.

Forgot to set nightly - it was reverted with the next commit

Copy link
Collaborator

@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.

Can you keep the feature just in the Cargo.toml, to avoid breakage for now? It will be removed in the next major bump (e.g. for '3.0').

@dbelv dbelv marked this pull request as ready for review May 9, 2023 14:31
@dbelv
Copy link
Contributor Author

dbelv commented May 9, 2023

Sorry I have overloaded the PR for clippy fixes as well

@@ -58,6 +58,3 @@ legacy = []

rustls = ["ethers-contract-abigen/rustls"]
openssl = ["ethers-contract-abigen/openssl"]

# Deprecated
eip712 = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add them back here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, yep.

@@ -70,6 +70,3 @@ rand.workspace = true
celo = ["legacy"] # celo support extends the transaction format with extra fields
legacy = []
macros = ["syn", "cargo_metadata", "once_cell"]

# Deprecated
eip712 = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same

Copy link
Collaborator

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

keep the flag in the Cargo.toml. Removing it will break compilation for library consumers

@@ -10,10 +10,6 @@ other utilities for interacting with the Ethereum ecosystem

For more information, please refer to the [book](https://gakonst.com/ethers-rs).

## Feature flags

- `eip712`: Provides the `Eip712` trait and derive procedural macro for EIP-712 encoding of typed data.
Copy link
Collaborator

Choose a reason for hiding this comment

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

modify to say "does nothing"

@@ -69,7 +69,6 @@ impl fmt::Display for Signature {
}
}

#[cfg(feature = "eip712")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

combine with main impl block

@dbelv dbelv changed the title Remove the eip712 feature flag and have it enabled by default. Fmt files Remove the eip712 feature flag and have it enabled by default May 10, 2023
Copy link
Collaborator

@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!

@DaniPopes DaniPopes merged commit 34f6a8c into gakonst:master May 10, 2023
13 of 15 checks passed
@dbelv dbelv deleted the eip712-remove branch July 16, 2023 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove eip712 feature
3 participants