-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add subxt
feature in subxt-signer
crate to default features
#1193
Add subxt
feature in subxt-signer
crate to default features
#1193
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! A nice little QoL tweak
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 👍
I wonder whether, to fix the feature flag compile issue, we should add no-default-features to the subxt dep in subxt-signer (it shouldn't care, after all, which subxt features are enabled). That might lead to other issues htough; needs some playing with! |
@@ -15,7 +15,7 @@ description = "Sign extrinsics to be submitted by Subxt" | |||
keywords = ["parity", "subxt", "extrinsic", "signer"] | |||
|
|||
[features] | |||
default = ["sr25519", "ecdsa"] | |||
default = ["sr25519", "ecdsa", "subxt", "native"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand this change; it means that signer by default will be "native" and then to be web you need to disable default features and then select eg subxt,web?
I think it'd be better to be consistent with subxt and ask the dependee library to pick which one it wants explicitly. This way, libraries can (and should) defer the choice unless it's relevant to them, and with features being additive, picking one by default also increases the odds that both end up enabled in a tree, breaking compilation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsdw My reasoning was that in 90% of cases people will just want to use it with native anyway. But you are right, I will change it to be similar to subxt in that an explicit choice has to be made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I get that; my main issue is that feature flags are additive and so the choice to enable web or native should be made at the last possible moment; enabling it by default makes it more likely that something depending on subxt somewhere in the tree and not thinking about features will screw up somebody trying to compile instead to web or whatever :)
Fixes #1189 .
Small PR to simplify use of subxt-signer to avoid confusion.