search

iancoleman / bip39

A web tool for converting BIP39 mnemonic codes
https://iancoleman.io/bip39/
MIT License
3.57k stars 1.45k forks source link

Investigate why Deimos is in the code #297

Closed iancoleman closed 5 years ago

iancoleman commented 5 years ago

deimos word didn't appear anywhere in the source code, no reference to it, and was crushing code execution when put my code under those lines,

See https://github.com/iancoleman/bip39/pull/296#issuecomment-466034468

Why is this unused code there?

Why is it 'crushing code execution' when it's supposedly unused?

skironDotNet commented 5 years ago

I don't know why it is in the code, but I know why it causes error.

deimos

  1. It does not necessarily break the execution of the whole, it will just stop execution of additional definitions in segwit-parameters.js if you put them under deimos.
  2. The reason the browser is throwing an error is simple... you load scripts in this order deimos1

Then the definition of deimos (whoever added it) in segwit-parameters.js deimos2

bitcoinjs-extensions.js deimos3

That's why I commented it out, I don't like garbage code, but I kept it as comments to preserve the values is someone spent the time to find them, but generally I would just delete it as there is no Deimos defined in index.js and bitcoinjs-extensions.js

But don't worry, I restored that code per your request, so you can merge #296

And finally to find out why this piece of code got there, do the code review (git commits history review) and check when by who or at what merge it got there

Good luck :)

iancoleman commented 5 years ago

Thanks for the info.

It was added in commit 3b6786bd51b2a339255689132af31691421e06c1 which is clean to revert, I'll do that. When it was merged I must have assumed deimos was already in the network list of bitcoinjs-extensions but it seems this was a false assumption.

In theory this could be resolved easily by copying the parameters from segwit-parameters into bitcoinjs-extensions but I'm not going maintaining code for the Deimos community. @Deimoscoin feel free to resubmit this coin via a new pull request including tests and all the required parameters. There are lots of examples in the commit history of what parts of the code to modify when adding new coins.

Thanks again for pointing this out.