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

Using identity's keystore as default first #677

Merged
merged 1 commit into from
Sep 24, 2019
Merged

Conversation

aphelionz
Copy link
Member

@aphelionz aphelionz commented Sep 3, 2019

This PR uses the keystore from options.identity first in createInstance, and then looks for options.keystore

Additionally, it marks a cache as "default" which means the Store class won't close it, as opposed to a custom cache location created by passing in options.directory, which it will close.

@aphelionz aphelionz mentioned this pull request Sep 5, 2019
@aphelionz aphelionz force-pushed the fix/createInstance branch 5 times, most recently from 1e3edf0 to f23527f Compare September 5, 2019 14:46
src/OrbitDB.js Outdated Show resolved Hide resolved
src/OrbitDB.js Outdated Show resolved Hide resolved
src/OrbitDB.js Outdated Show resolved Hide resolved
src/OrbitDB.js Outdated Show resolved Hide resolved
src/OrbitDB.js Outdated Show resolved Hide resolved
src/OrbitDB.js Outdated Show resolved Hide resolved
@aphelionz aphelionz force-pushed the fix/createInstance branch 3 times, most recently from 588765e to 7eedd43 Compare September 10, 2019 15:20
@aphelionz
Copy link
Member Author

@haadcode I believe all the tests should pass now

@aphelionz aphelionz force-pushed the fix/createInstance branch 2 times, most recently from 4000379 to a2c3fa4 Compare September 13, 2019 20:10
@aphelionz
Copy link
Member Author

Ok, pushed some more requested changed.

@haadcode @shamb0t Please have a look. I'm too deep in the weeds to think about it clearly / objectively and some of the requested changes require changes to our semantics around drop and close

You can see examples of this in the tests and also in this orbit-db-cache PR: orbitdb-archive/orbit-db-cache#14

src/OrbitDB.js Outdated Show resolved Hide resolved
src/OrbitDB.js Outdated Show resolved Hide resolved
@shamb0t
Copy link
Member

shamb0t commented Sep 17, 2019

@aphelionz can you provide tests which fail that this PR fixes? It's still not clear to me that this is necessary. Are these essentially keystore and cache managers? Is it inconsistent to expect users to close cache/keystore themselves if they are non-default ones passed in?

Also can you elaborate on

some of the requested changes require changes to our semantics around drop and close

What are the requested changes and what are the changes to the semantics around drop and close?

src/OrbitDB.js Outdated Show resolved Hide resolved
src/OrbitDB.js Outdated Show resolved Hide resolved
package-lock

pointing to branch

more cache management stuff

WIP

Passing tests

Removing static linking

fixing tests and linting

fixing package.json

removing last debugger

removing last debugger

Adding keystore and cache getters

PR comments

Removing extraneous cache management

Package files

Closing caches

using dbAddress as this.caches key

new tests for store management

Working but with slightly different semantics

Rebuild

package-lock

Dependency updates

removeHandler

restoring db.close in replication status test

package.json files

move handler to orbitdb.caches

Test updates

Cache management cleanup

use store.options.directory

requestCache in onLoad and onDrop

add status test

Adding db to this.stores in onLoad and onDrop

Working RC5 before rebase

Updating package-lock

restoring original replicaiton status test

package files

removing keystore getter

more keystore cleanup

typo
@aphelionz aphelionz merged commit 1344c8a into master Sep 24, 2019
@aphelionz aphelionz deleted the fix/createInstance branch September 24, 2019 15:06
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.

None yet

4 participants