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

OpenAlias: Plugin v0.1 #986

Merged
merged 20 commits into from
Feb 17, 2015
Merged

OpenAlias: Plugin v0.1 #986

merged 20 commits into from
Feb 17, 2015

Conversation

fluffypony
Copy link
Contributor

Implements basic resolution, email-style addresses, optional auto-add to contacts, and full DNSSEC trust-chain verification.

Still to do: implement optional support for the OA resolver set, add DNSCrypt support

@ecdsa
Copy link
Member

ecdsa commented Jan 24, 2015

thanks a lot.
I will try to test it this week end

@fluffypony
Copy link
Contributor Author

One point for discussion is the timeouts I've set are quite "aggressive". Timeout for initial resolve of the address is 2 seconds, and timeout for the DNSSEC trust chain is 1 second per UDP request. I based that on my real-life example (South African ADSL + wifi, as well as 3G + wifi) and consistently had UDP pings returning from 8.8.8.8 in <600ms. That having been said, its entirely possible that this may be too aggressive as far as timeouts go, we'll have to see how the general Electrum populace responds.

I'd also like to motivate for this plugin to be enabled by default, assuming there are no security concerns in the code, as it will be invisible to existing users but will create a really seamless experience when people start using it.

@fluffypony
Copy link
Contributor Author

@ecdsa updated to match the old alias system's behaviour, merged with upstream for your convenience

@ecdsa
Copy link
Member

ecdsa commented Feb 17, 2015

please do not add new dependencies (dnspython).
use is_available() to disable your plugin if the dependencies are not satisfied

@fluffypony
Copy link
Contributor Author

@ecdsa Are you being serious? You mean the dependency that I asked you if we could add?

[2015-01-15T17:53:57+0200] <fluffypony> ok and once tested against that are you ok with me submitting a PR? it does add a new dependency (dnspython) to Electrum
[2015-01-15T17:55:27+0200] <ThomasV> new dependencies are ok if i's for a plugin
[2015-01-15T17:55:41+0200] <ThomasV> but if you can get rid of them it's better of course
[2015-01-15T17:56:37+0200] <fluffypony> can't get rid of it without implementing DNSSEC trust chain stuff from scratch ;)

Not having the dependency included is untenable for the plugin. This is a plugin designed to improve the usability of Electrum (and of Bitcoin by extension). If it is unavailable by default in official builds and only available if the user jumps through hoops and compiles Electrum themselves then that is entirely pointless. Moreover, not including the dependency means that OpenAlias will be unavailable on Android.

The target demographic for OpenAlias isn't the Bitcoin early adopter who has memorised their address, it's for newcomers, Windows users, and mobile users who want Bitcoin to be more usable.

Oh, and to your second point, we already do check if the dependency is available and set is_available() accordingly: https://github.com/openalias/electrum/blob/master/plugins/openalias.py#L71 and https://github.com/openalias/electrum/blob/master/plugins/openalias.py#L82

@ecdsa
Copy link
Member

ecdsa commented Feb 17, 2015

Sorry for the misunderstanding: the point of the plugin system is to avoid overloading the code with new dependencies. Electrum should be as portable as possible, and adding dependencies reduces portability.
This does not mean that the dependencies will not be added to official binaries, or to android versions; for example, the current beta binaries include support for trezor.

@fluffypony
Copy link
Contributor Author

@ecdsa ok - so then besides reverting the dependency is there anything else that needs doing?

@ecdsa
Copy link
Member

ecdsa commented Feb 17, 2015

thanks! I tested this morning, it looked good. don't worry about travis complaining, that's because of me.

ecdsa added a commit that referenced this pull request Feb 17, 2015
@ecdsa ecdsa merged commit 42e586d into spesmilo:master Feb 17, 2015
@ecdsa
Copy link
Member

ecdsa commented Feb 17, 2015

finally, I included dnspython in setup.py. since it is pure-python, I guess we can take that risk

marceloneil pushed a commit to marceloneil/electrum that referenced this pull request Mar 28, 2019
Warn users when entering a BIP38 key that Electron Cash does not support such keys.  This is so they don't get mad at Electron Cash pretending like BIP38 doesn't exist and at least get an intelligible error message.

In the future we really want to enable BIP38 but that requires Python 3.6 + openssl 1.1 which presents a packaging obstacle for OSX due to pyenv bugs.  TODO! FIX!
Toporin pushed a commit to Toporin/electrum-satochip that referenced this pull request Feb 16, 2021
Warn users when entering a BIP38 key that Electron Cash does not support such keys.  This is so they don't get mad at Electron Cash pretending like BIP38 doesn't exist and at least get an intelligible error message.

In the future we really want to enable BIP38 but that requires Python 3.6 + openssl 1.1 which presents a packaging obstacle for OSX due to pyenv bugs.  TODO! FIX!
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.

None yet

3 participants