Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[CO-373] Move wallet@Pos.Util.Mnemonic to wallet-new@Cardano.Wallet.Kernel.BIP39 #3663

Merged
merged 4 commits into from
Sep 28, 2018

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Sep 25, 2018

Description

The old wallet code should be removed. This pr moves Mnemonic from wallet to wallet-new as BIP39.

At the same time I replaced the TestVector tuple with record wildcards in the contained BIP39Spec.hs. Feel free to disagree.

Note Probably easiest to view changes per commit rather than all at once. The first commit merely moves/copies, while subsequent commits make the important changes.

Would make sense to squash everything later.

Depends on #3667.

Linked issue

CO-373

Type of change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • 🛠 New feature (non-breaking change which adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • 🔨 New or improved tests for existing code
  • ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

Screenshots (if available)

@Anviking Anviking requested a review from KtorZ September 25, 2018 12:05
@Anviking
Copy link
Member Author

Anviking commented Sep 25, 2018

Actually, I've broken stuff in cardano-sl-faucet Investigating

I've missed

faucet/src/Cardano/Faucet/Init.hs:import           Pos.Util.Mnemonic (Mnemonic, entropyToMnemonic, genEntropy)
faucet/src/Cardano/Faucet/Types/Config.hs:import           Pos.Util.Mnemonic (Mnemonic)
scripts/generate-mnemonic.hs:import           Pos.Util.Mnemonic (EntropySize, Mnemonic, entropyToMnemonic,
tools/src/Pos/Tools/Dbgen/Lib.hs:import           Pos.Util.Mnemonic (Mnemonic, entropyToMnemonic, genEntropy)
wallet-new/bench/Client/Cardano/Wallet/Web/Endpoint/NewWallet.hs:import           Pos.Util.Mnemonic (Mnemonic, entropyToMnemonic, genEntropy)

fixing

Hm, Dbgen might be a bit tricky…

initMeta <- V0.CWalletMeta <$> pure newwalName
<*> migrate newwalAssuranceLevel
<*> pure 0
let walletInit = V0.CWalletInit initMeta (V0.CBackupPhrase backupPhrase)
let walletInit = V0.CWalletInit initMeta $ error "TODO: File should be removed"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

Copy link
Member Author

Choose a reason for hiding this comment

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

We then get problems since wallet and wallet-new are using different Mnemonic types.

/Users/Johannes/IOHK/cardano-sl/wallet-new/src/Cardano/Wallet/API/V1/LegacyHandlers/Wallets.hs:93:64: error:
     Couldn't match expected type Pos.Util.Mnemonic.Mnemonic 12
                  with actual type Cardano.Wallet.Kernel.BIP39.Mnemonic 12
      NB: Cardano.Wallet.Kernel.BIP39.Mnemonic is defined at
            src/Cardano/Wallet/Kernel/BIP39.hs:(84,1)-(87,25)
          Pos.Util.Mnemonic.Mnemonic
            is defined in Pos.Util.Mnemonic
                in package cardano-sl-wallet-1.3.0
     In the first argument of V0.CBackupPhrase, namely
        backupPhrase
      In the second argument of V0.CWalletInit, namely
        (V0.CBackupPhrase backupPhrase)
      In the expression:
        V0.CWalletInit initMeta (V0.CBackupPhrase backupPhrase)
   |
93 |     let walletInit = V0.CWalletInit initMeta (V0.CBackupPhrase backupPhrase)
   |                                                                ^^^^^^^^^^^^

Copy link
Contributor

Choose a reason for hiding this comment

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

Meh. That's exactly why I wanted to remove all those modules first :/ ...
Fine then, but let's only replace the minimum to make stuff compile and wait for the external handlers to be moved

@Anviking Anviking force-pushed the anviking/CO-373/move-mnemonic branch 3 times, most recently from 1c9b048 to 8d73b2f Compare September 27, 2018 13:48
@KtorZ KtorZ changed the base branch from develop to CO-372/TheGreatCleanup September 27, 2018 15:30
Some of these must be revisited at a later stage
- Remove references to V0 API
- Use RecordWildCards instead of huge tuple
Copy link
Contributor

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

LGTM overall.

Few remarks about the commits though:

  • 00b3e20 and 2db81b2 should be squashed together
  • 3e39033 should have a proper description explaining the reason of deletion (and why it is safe / done at this moment).

Will do while merging.

runTests
[ Test.Pos.Tools.Dbgen.Json.tests
]
runTests []
Copy link
Contributor

Choose a reason for hiding this comment

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

runTests [] 😅 ... talking about useless tests huh?
No big deal though.

@KtorZ KtorZ merged commit 9f0f36e into CO-372/TheGreatCleanup Sep 28, 2018
@KtorZ KtorZ deleted the anviking/CO-373/move-mnemonic branch September 28, 2018 09:06
KtorZ added a commit that referenced this pull request Sep 28, 2018
…emonic

[CO-373] Move `wallet@Pos.Util.Mnemonic` to `wallet-new@Cardano.Wallet.Kernel.BIP39`
KtorZ added a commit that referenced this pull request Oct 1, 2018
…emonic

[CO-373] Move `wallet@Pos.Util.Mnemonic` to `wallet-new@Cardano.Wallet.Kernel.BIP39`
@KtorZ KtorZ mentioned this pull request Oct 1, 2018
12 tasks
KtorZ added a commit that referenced this pull request Oct 2, 2018
…emonic

[CO-373] Move `wallet@Pos.Util.Mnemonic` to `wallet-new@Cardano.Wallet.Kernel.BIP39`
KtorZ added a commit that referenced this pull request Oct 3, 2018
…emonic

[CO-373] Move `wallet@Pos.Util.Mnemonic` to `wallet-new@Cardano.Wallet.Kernel.BIP39`
KtorZ added a commit that referenced this pull request Oct 4, 2018
…emonic

[CO-373] Move `wallet@Pos.Util.Mnemonic` to `wallet-new@Cardano.Wallet.Kernel.BIP39`
KtorZ added a commit that referenced this pull request Oct 8, 2018
…emonic

[CO-373] Move `wallet@Pos.Util.Mnemonic` to `wallet-new@Cardano.Wallet.Kernel.BIP39`
KtorZ added a commit that referenced this pull request Oct 11, 2018
KtorZ added a commit that referenced this pull request Oct 22, 2018
KtorZ added a commit that referenced this pull request Oct 31, 2018
KtorZ added a commit that referenced this pull request Nov 2, 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.

2 participants