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

[CO-430] Cleanup cardano-sl references to cardano-sl-wallet #3750

Merged
merged 1 commit into from
Oct 18, 2018
Merged

[CO-430] Cleanup cardano-sl references to cardano-sl-wallet #3750

merged 1 commit into from
Oct 18, 2018

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Oct 15, 2018 •
edited
Loading

Description

Removes references to cardano-sl-wallet.

TODO:

  • Sanity check / commit messages

Side note on status

Quite a lot are remaining, but they should disappear after wallet/ is removed.

From this pr:

git grep "cardano-sl-wallet" | grep -v "wallet-new"
docs/how-to/build-cardano-sl-and-daedalus-from-source-code.md:-  `cardano-sl-wallet-static`:
pkgs/default.nix:"cardano-sl-wallet" = callPackage
pkgs/default.nix:pname = "cardano-sl-wallet";
pkgs/default.nix:, cardano-sl-wallet
pkgs/default.nix:cardano-sl-wallet
pkgs/default.nix:cardano-sl-wallet
pkgs/default.nix:cardano-sl-wallet
pkgs/default.nix:cardano-sl-wallet
pkgs/default.nix:"cardano-sl-wallet-test" = callPackage
pkgs/default.nix:, cardano-sl-wallet
pkgs/default.nix:pname = "cardano-sl-wallet-test";
pkgs/default.nix:cardano-sl-wallet
stack.yaml:  cardano-sl-wallet:            -Wall -Werror -Wcompat -fwarn-redundant-constraints
wallet/Makefile:	    --command "stack ghci cardano-sl-wallet --ghci-options=-fno-code"
wallet/Makefile:	    --command "stack ghci cardano-sl-wallet:lib cardano-sl-wallet:test:cardano-wallet-test --ghci-options=-fobject-code" \
wallet/README.md:# cardano-sl-wallet
wallet/cardano-sl-wallet.cabal:name:                cardano-sl-wallet
wallet/cardano-sl-wallet.cabal:                     , cardano-sl-wallet
wallet/test/cardano-sl-wallet-test.cabal:name:                cardano-sl-wallet-test
wallet/test/cardano-sl-wallet-test.cabal:                     , cardano-sl-wallet

Only non-trivial thing left should be:

stack.yaml:  cardano-sl-wallet:            -Wall -Werror -Wcompat -fwarn-redundant-constraints

Linked issue

CO-428

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 added the wip label Oct 15, 2018
@Anviking Anviking changed the base branch from develop to CO-372/TheGreatCleanup October 15, 2018 10:56
@Anviking Anviking force-pushed the anviking/CO-428/rm-cardano-sl-wallet branch 5 times, most recently from 6090c17 to 45e4401 Compare October 16, 2018 12:07
@Anviking Anviking changed the title Cleanup cardano-sl references to cardano-sl-wallet [CO-428] Cleanup cardano-sl references to cardano-sl-wallet Oct 16, 2018
@Anviking Anviking force-pushed the anviking/CO-428/rm-cardano-sl-wallet branch 2 times, most recently from a6768ed to 23a09a7 Compare October 16, 2018 18:45
@Anviking Anviking removed the wip label Oct 16, 2018
@Anviking Anviking changed the title [CO-428] Cleanup cardano-sl references to cardano-sl-wallet [CO-430] Cleanup cardano-sl references to cardano-sl-wallet Oct 16, 2018
@Anviking Anviking requested a review from KtorZ October 16, 2018 18:48
Anviking
Anviking commented Oct 17, 2018
View reviewed changes
nix/overlays/required.nix Outdated
@@ -31,7 +31,7 @@ self: super: {
inherit enableProfiling;
};
});
cardano-sl-wallet-static = justStaticExecutablesGitRev super.cardano-sl-wallet;
cardano-sl-wallet-static = justStaticExecutablesGitRev super.cardano-sl-wallet-new;
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this is the iffy part of this

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this can just go in favor of the line below:

 cardano-sl-wallet-new-static = justStaticExecutablesGitRev super.cardano-sl-wallet-new;

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooohh 🤦‍♂️

@Anviking Anviking force-pushed the anviking/CO-428/rm-cardano-sl-wallet branch from 23a09a7 to f70f4b6 Compare October 17, 2018 08:24
KtorZ
KtorZ reviewed Oct 17, 2018
View reviewed changes
appveyor.yml
@@ -80,7 +80,6 @@ before_test:
# After some investigation, it was discovered that this was because 'rocksdb.dll' has to be located in this folder as well, or else the test executable doesn't work.
- copy rocksdb.dll node
- copy rocksdb.dll lib
- copy rocksdb.dll wallet
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this isn't still needed to compile the wallet on windows :o

Copy link
Member Author

Choose a reason for hiding this comment

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

Aren't there tests for that?

@KtorZ
Copy link
Contributor

KtorZ commented Oct 17, 2018

I believe we can also remove stuff from faucet right? It only imports the wallet for the Pos.Util.Mnemonic module that can be switched for the one in the wallet-new

@Anviking
Copy link
Member Author

Anviking commented Oct 17, 2018 •
edited
Loading

@KtorZ fixed in 04f707c, and here the dependency is removed in the faucet/cardano-sl-faucet.cabal.

Had memory of fixing it here as well. Makes sense that it disappeared when rebasing though.

@Anviking
[CO-430] Remove occurrences of cardano-sl-wallet
49b1e6e 
The goal of CO-372 is to remove the old wallet. This commit removes it
from various nix, CI and cabal files without any immediate trouble.
@Anviking Anviking force-pushed the anviking/CO-428/rm-cardano-sl-wallet branch from f70f4b6 to 49b1e6e Compare October 17, 2018 08:41
Anviking
Anviking commented Oct 17, 2018
View reviewed changes
docs/how-to/build-cardano-sl-and-daedalus-from-source-code.md
@@ -79,7 +79,7 @@ NOTE: the various other Cardano components can be obtained through other attribu
- `cardano-explorer`, `cardano-explorer-swagger`, `cardano-explorer-mock`
- `cardano-sl-tools`:
- `cardano-analyzer`, `cardano-dht-keygen`, `cardano-genupdate`, `cardano-keygen`, `cardano-launcher`, `cardano-addr-convert`, `cardano-cli-docs`, `cardano-block-gen`, `cardano-post-mortem`
- `cardano-sl-wallet-static`:
- `cardano-sl-wallet-new-static`:
Copy link
Member Author

@Anviking Anviking Oct 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is a problem now when cardano-sl-wallet-static is removed.

Is it fine/correct in the context to replace with cardano-sl-wallet-new-static as I've done here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose 😶

KtorZ
KtorZ approved these changes Oct 18, 2018
View reviewed changes
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

@KtorZ KtorZ merged commit f9250d0 into CO-372/TheGreatCleanup Oct 18, 2018
@KtorZ KtorZ deleted the anviking/CO-428/rm-cardano-sl-wallet branch October 18, 2018 16:37
@KtorZ KtorZ mentioned this pull request Oct 22, 2018
Merged
12 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KtorZ KtorZ KtorZ approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Anviking @KtorZ