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

Better structure for the internal organization of our state directory #566

Merged
merged 9 commits into from
Jul 23, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Jul 18, 2019

Issue Number

#564

Overview

  • I have fixed an issue where the parent directories of --state-dir wouldn't be created and fail in a not-so-user-friendly way:
$ cardano-wallet-http-bridge launch --state-dir /tmp/a/b/c
cardano-wallet-http-bridge: /tmp/a/b/c: createDirectory: does not exist (No such file or directory)
  • I have now used non-overlapping namespaces for storing chain's data for the different backends

Comments

$ tree ~/.cardano-wallet/
/home/ktorz/.cardano-wallet/
├── httpbridge
│   ├── mainnet
│   │   ├── mainnet
│   │   │   ├── blob
│   │   │   ├── chainstate
│   │   │   ├── config.yml
│   │   │   ├── epoch
│   │   │   ├── index
│   │   │   ├── pack
│   │   │   ├── refpack
│   │   │   └── tag
│   │   ├── wallet.db
│   │   ├── wallet.db-shm
│   │   └── wallet.db-wal
│   └── testnet
│       ├── testnet
│       │   ├── blob
│       │   ├── chainstate
│       │   ├── config.yml
│       │   ├── epoch
│       │   ├── index
│       │   ├── pack
│       │   ├── refpack
│       │   └── tag
│       ├── wallet.db
│       ├── wallet.db-shm
│       └── wallet.db-wal
└── jormungandr
    └── testnet
        ├── chain
        │   ├── blocks.sqlite
        │   ├── blocks.sqlite-shm
        │   └── blocks.sqlite-wal
        ├── jormungandr-config.json
        ├── wallet.db
        ├── wallet.db-shm
        └── wallet.db-wal

@KtorZ KtorZ self-assigned this Jul 18, 2019
@KtorZ KtorZ requested a review from piotr-iohk July 18, 2019 13:17
Copy link
Contributor

@piotr-iohk piotr-iohk left a comment

Choose a reason for hiding this comment

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

lgtm 👍
(but tests need to be adjusted)

rvl
rvl previously requested changes Jul 19, 2019
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

I like that parent directories are created and there are by default separate state directories for each network of each backend.

But nesting 2 levels of subdirectories within the state dir will make deployment less convenient. We are also committing the same error as http-bridge does -- see how that causes an unnecessary nesting of directories.

I think we should change it to stateDirOption :: Parser (Maybe FilePath). Adjust the help string to say the default is $HOME/.cardano-wallet/BACKEND/$network. Then fill in the default value if the option was Nothing.

lib/cli/src/Cardano/CLI.hs Outdated Show resolved Hide resolved
lib/cli/src/Cardano/CLI.hs Outdated Show resolved Hide resolved
@piotr-iohk
Copy link
Contributor

Meanwhile, I've adjusted the tests and added tests for nested dir state-dir.

KtorZ and others added 6 commits July 22, 2019 18:36
Such that our state dir looks nicer:

/home/ktorz/.cardano-wallet/
└── jormungandr
    └── testnet
        ├── chain
        │   ├── blocks.sqlite
        │   ├── blocks.sqlite-shm
        │   └── blocks.sqlite-wal
        ├── jormungandr-config.json
        ├── wallet.db
        ├── wallet.db-shm
        └── wallet.db-wal
@rvl rvl force-pushed the KtorZ/state-dir-namespace branch from df534c3 to 4c18851 Compare July 22, 2019 08:38
- Put the default --state-dir within the XDG data dir
- Make --state-dir point to the actual state dir, rather than nesting
  directories within.
@rvl rvl force-pushed the KtorZ/state-dir-namespace branch from 4c18851 to ceaf74c Compare July 22, 2019 11:32
@rvl rvl dismissed their stale review July 22, 2019 11:34

Updated

We do use 'mempty' often to align text and ease readability. So the small overhead is acceptable
and, most probably removed by GHC at compile-time...
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.

3 participants