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

save app params #65

Merged
merged 1 commit into from
Jun 28, 2024
Merged

save app params #65

merged 1 commit into from
Jun 28, 2024

Conversation

turbocrime
Copy link
Contributor

@turbocrime turbocrime commented Jun 26, 2024

at some point saving app params to local was removed.

this restores ability to operate offline.

edit: fixes #32
accidentally duplicating work in #64

@turbocrime turbocrime requested a review from a team June 26, 2024 19:10
Copy link

changeset-bot bot commented Jun 26, 2024

🦋 Changeset detected

Latest commit: 6249c29

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
chrome-extension Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Valentine1898
Copy link
Contributor

It seems that this solution, as well as #64, does not handle the case where the chain-id can change without changing the RPC

@turbocrime
Copy link
Contributor Author

turbocrime commented Jun 27, 2024

chain-id should not change for the lifetime of the chain. if chain-id changes without changing the rpc, the rpc is serving a new chain. at this point all local state is invalid.

since chain-id is part of idb name in the storage package

https://github.com/penumbra-zone/web/blob/8388020a17e42efaf69c5d6526fbb309f72f4291/packages/storage/src/indexed-db/index.ts#L92

services will connect to a (probably new) idb database correct for the chain, and begin sync. this is equivalent to clearing the cache, but the old chain-id state remains available the old database name.

there is not really a correct solution to this unless idb connection names are changed

@Valentine1898
Copy link
Contributor

All the time we had only one RPC for the testnet, while the chain-id changed many times. If RPC starts serving a new chain, then when indexed-db is initialized, the chain-id from local storage (old chain) will be used. This means that the data of the new chain will be written to the same database on top of the old data, and this will probably lead to various bugs

@turbocrime
Copy link
Contributor Author

you're correct, sorry. i had some logic previously in sercices-context init that i believe was comprehensive

@turbocrime
Copy link
Contributor Author

on phone rn and difficult to link properly but ab9d743 of penumbra-zone web should contain something usable that could be copied from services-context index

@Valentine1898
Copy link
Contributor

We can actually prioritize remote params, and only if they are not available use locally stored params

Copy link
Contributor

@Valentine1898 Valentine1898 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 it seems that extension without network doesn't work as expected (you can't see your balances)

@turbocrime turbocrime merged commit bc51c5c into main Jun 28, 2024
3 checks passed
@turbocrime turbocrime deleted the turbocrime/save-params branch June 28, 2024 06:45
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.

'params' are never stored in local storage
2 participants