Skip to content
This repository has been archived by the owner on Feb 14, 2021. It is now read-only.

Add the kip6 feature gate #15

Merged
merged 2 commits into from
Aug 21, 2018
Merged

Add the kip6 feature gate #15

merged 2 commits into from
Aug 21, 2018

Conversation

lexfrl
Copy link
Contributor

@lexfrl lexfrl commented Aug 21, 2018

I'd like to make kip4 as a default as well, but don't sure

Cargo.toml Outdated
kip4 = []
std = ["pwasm-std/std", "parity-hash/std", "uint/std", "byteorder/std"]
kip6 = []
std = ["kip6", "pwasm-std/std", "parity-hash/std", "uint/std", "byteorder/std"]
Copy link
Contributor

Choose a reason for hiding this comment

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

hm could you explain why std requires kip6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why not? std feature is a default, but with std

Cargo.toml Outdated
@@ -30,6 +30,7 @@ version = "1.2.2"
default-features = false

[features]
default = []
default = ["kip6"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be default only after the fork?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's make sense. The binaries will not change until user will use it (it'll optimized out). So the final decision will be still in hands of user and it will depend only on the code user wrote, regardless if kip6 feature is enabled or disabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't see any point in feature gate if it is on by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see either. That's why I had with question openethereum/parity-ethereum#9357 (comment)

Maybe some people would consider it useful, but I can't imagine what kind of case that would be.

Copy link
Contributor Author

@lexfrl lexfrl Aug 21, 2018

Choose a reason for hiding this comment

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

I don't like the idea to force users to pass this feature for no reason in every contract as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

They won't need to pass it until the fork is done with the exception if they use custom chain spec.

And after the fork, it will be the default, so they won't need to pass it again, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the idea behind make it the "default" feature is that by "default" user don't have to care either pwasm-ethereum has this extern or not (see #15 (comment))

Copy link
Collaborator

Choose a reason for hiding this comment

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

"default" configuration is the kind of configuration that won't allow a user to generate invalid binaries for kovan network

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And after the fork, it will be the default, so they won't need to pass it again, right?

I just think that we have to care about this, because again:

it will depend only on the code user wrote, regardless if kip6 feature is enabled or disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"default" configuration is the kind of configuration that won't allow a user to generate invalid binaries for kovan network

ok, that's a good argument.

@lexfrl lexfrl changed the title Add the kip6 feature gate (make it default) Add the kip6 feature gate Aug 21, 2018
@lexfrl lexfrl merged commit 85c909e into master Aug 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants