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.

Sign up for GitHub

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

Jump to bottom

Make storage not dependent on WASM #499

Merged
merged 1 commit into from May 16, 2023
Merged

Make storage not dependent on WASM #499

merged 1 commit into from May 16, 2023

Conversation

benthecarman
Copy link
Collaborator

Need to add docs and more tests but this works ™️

@benthecarman benthecarman force-pushed the wasm-rip branch 2 times, most recently from 06be03f to ef505ea Compare May 11, 2023 15:39
TonyGiorgio
TonyGiorgio reviewed May 11, 2023
View reviewed changes
Copy link
Contributor

@TonyGiorgio TonyGiorgio left a comment

Choose a reason for hiding this comment

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

I'll need to do a deeper dive later, but really nice work. I like the direction this PR takes things, so I don't think I'll have any structural change requests later. Just a few comments about some things.

mutiny-wasm/src/lib.rs Show resolved Hide resolved
mutiny-core/src/storage.rs Show resolved Hide resolved
mutiny-core/src/storage.rs Show resolved Hide resolved
@benthecarman benthecarman force-pushed the wasm-rip branch 3 times, most recently from 1c512f3 to 983abd8 Compare May 15, 2023 16:35
@TonyGiorgio
Copy link
Contributor

needs rebase

TonyGiorgio
TonyGiorgio reviewed May 15, 2023
View reviewed changes
mutiny-wasm/src/indexed_db.rs
}

async fn build_indexed_db_database() -> Result<Rexie, MutinyError> {
let rexie = Rexie::builder(WALLET_DATABASE_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

So now everything will be saved into WALLET_DATABASE_NAME but as a key value store? So the gossip, logging, and peer storage stuff will go away? Kind of a breaking change except those components were not critical right?

Do all the keys in the wallet DB map exactly the same with this PR too? Trying to figure out if all clients will need a fresh instance after this upgrade.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So the gossip, logging, and peer storage stuff will go away? Kind of a breaking change except those components were not critical right?

yeah pretty much

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems so. We just have to reconnect peer. I'll make sure to test that.

TonyGiorgio
TonyGiorgio requested changes May 15, 2023
View reviewed changes
mutiny-core/src/storage.rs Show resolved Hide resolved
mutiny-core/src/storage.rs Show resolved Hide resolved
mutiny-core/src/auth.rs Show resolved Hide resolved
mutiny-core/src/auth.rs Show resolved Hide resolved
mutiny-core/src/gossip.rs Outdated Show resolved Hide resolved
mutiny-core/src/logging.rs Outdated Show resolved Hide resolved
mutiny-core/src/nodemanager.rs Outdated Show resolved Hide resolved
mutiny-core/src/peermanager.rs Show resolved Hide resolved
mutiny-wasm/src/indexed_db.rs Show resolved Hide resolved
@benthecarman benthecarman force-pushed the wasm-rip branch 3 times, most recently from 386aece to b6021e6 Compare May 16, 2023 19:39
@TonyGiorgio
Copy link
Contributor

Okay I think the code looks good, massive work. Gonna spend a bit of time testing this.

@TonyGiorgio
Copy link
Contributor

TonyGiorgio commented May 16, 2023 •
edited 

panicked at 'Error reading ciphertext: labor badge orient giant answer ...', mutiny-core/src/encrypt.rs:32:29

@TonyGiorgio
Copy link
Contributor

I did an export and import and it didn't look like everything came through.

Here's what I got after importing:

auth_profiles
fee_estimates
keychain_store
last_sync_timestamp
ln_peer/
logs
manager_
mnemonic
network_graph
nodes

But I'm missing a handful of things and it looks like the mnemoic is different so I don't think this works very well. Hard to diagnose what might have gone wrong.

The export file looks right but it's hard to parse because logs are in here too, I think we should be excluding that.

@benthecarman benthecarman force-pushed the wasm-rip branch 2 times, most recently from c0725b3 to 6c20352 Compare May 16, 2023 22:18
TonyGiorgio
TonyGiorgio approved these changes May 16, 2023
View reviewed changes
Copy link
Contributor

@TonyGiorgio TonyGiorgio left a comment

Choose a reason for hiding this comment

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

LFG

@benthecarman benthecarman merged commit 9a10748 into master May 16, 2023
3 checks passed
@benthecarman benthecarman deleted the wasm-rip branch May 16, 2023 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TonyGiorgio TonyGiorgio TonyGiorgio 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.

None yet
2 participants
@benthecarman @TonyGiorgio