Skip to content

refactor: Remove bip39 library#86

Merged
faustbrian merged 6 commits intoArkEcosystemArchive:masterfrom
sleepdefic1t:fix/remove-bip39-lib
May 25, 2019
Merged

refactor: Remove bip39 library#86
faustbrian merged 6 commits intoArkEcosystemArchive:masterfrom
sleepdefic1t:fix/remove-bip39-lib

Conversation

@sleepdefic1t
Copy link
Contributor

@sleepdefic1t sleepdefic1t commented May 20, 2019

Proposed changes

bip39-lib has several bugs as discussed in #81 & #72

This PR removes the mnemonic class, tests, documentation, examples, and lib dependency (bip39).

It is also a breaking change and bumps C++ Crypto to v1.0.0

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes

@sleepdefic1t
fix: remove bip39 library & mnemonic feature
90bea17 
bip39-lib has several critical bugs discussed in #81

This PR removes the mnemonic class, references, tests, documentation and dependency (bip39-lib).
@ghost ghost added Complexity: High More than 256 lines changed. Type: Bugfix The pull request fixes an incorrect functionality or behaviour. labels May 20, 2019
@ghost
Copy link

ghost commented May 20, 2019 •
edited by ghost
Loading

The ci/circleci: build-linux-clang-5 job is failing as of 6f605db1ab7910ceea6dc69199b6ebe959310744. Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@sleepdefic1t sleepdefic1t changed the title fix: Remove bip39 library [WIP]fix: Remove bip39 library May 20, 2019
@sleepdefic1t
Copy link
Contributor Author

sleepdefic1t commented May 21, 2019

Looks like there are additional build issues with ESP-8266 with PlatformIO on Linux-only.

I can confirm that code builds and flashes to an ESP8266, apparently just not on Linux atm.

Will investigate more and decide what to do with this PR.

[WIP] for now.

@faustbrian
Copy link
Contributor

faustbrian commented May 21, 2019

1.0.0 is reserved for the first stable release of all crypto packages when AIP11 is implemented in all of them.

faustbrian
faustbrian reviewed May 21, 2019
View reviewed changes
@sleepdefic1t sleepdefic1t changed the title [WIP]fix: Remove bip39 library fix: Remove bip39 library May 21, 2019
@sleepdefic1t
Copy link
Contributor Author

sleepdefic1t commented May 21, 2019

@faustbrian
Should be good to go now.
Thank you.

This was referenced May 21, 2019
Closed
Closed
@sleepdefic1t
Copy link
Contributor Author

sleepdefic1t commented May 22, 2019

@faustbrian

This is a breaking change, but it’s ready.
Can we merge with a minor bump,
or are we waiting until 1.0?

@faustbrian
Copy link
Contributor

faustbrian commented May 24, 2019

We can release this with a minor bump, everything before 1.0.0 should be seen as unstable and can have breaking changes any time.

@sleepdefic1t
Copy link
Contributor Author

sleepdefic1t commented May 24, 2019

🤔
Nothing in this PR should affect that failing CI test in build-linux-clang-5.
#92 passed with the the same transaction class and tests (circleci/ArkEcosystem/cpp-crypto/713).

You can rebuild if you want to double-check, @faustbrian.
Otherwise, this should be good to go 👍

@faustbrian faustbrian changed the title fix: Remove bip39 library refactor: Remove bip39 library May 25, 2019
@faustbrian faustbrian merged commit 8396209 into ArkEcosystemArchive:master May 25, 2019
@sleepdefic1t sleepdefic1t deleted the fix/remove-bip39-lib branch May 27, 2019 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kristjank kristjank Awaiting requested review from kristjank

@spkjp spkjp Awaiting requested review from spkjp

1 more reviewer

@faustbrian faustbrian faustbrian approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Complexity: High More than 256 lines changed. Type: Bugfix The pull request fixes an incorrect functionality or behaviour.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sleepdefic1t @faustbrian