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

feat: move the algod client to store #224

Merged
merged 2 commits into from
Sep 3, 2024
Merged

feat: move the algod client to store #224

merged 2 commits into from
Sep 3, 2024

Conversation

acfunk
Copy link
Contributor

@acfunk acfunk commented Aug 29, 2024

This is to address issue #213

  • replaced the algodClient prop on the WalletManager with a getter and setter that point to the store
  • removed algodClient from persisted storage - the re-serialization makes it useless anyway
  • setActiveNetwork on store now takes algodClient as a parameter
  • tests were updated to reflect the changes

Copy link
Collaborator

@drichar drichar left a comment

Choose a reason for hiding this comment

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

Appreciate you splitting this from your other PR so the changes can be addressed separately. Overall this looks good, and most of my change requests are minor optimizations. The important one is making sure loadPersistedState has the correct return type.

While this does handle moving the algod client to the store in the core library, I'm curious whether this now works as expected in your Vue app (AlgoTools)? When @SilentRhetoric made his PR to fix network switching in the Solid adapter, the critical change was moving the algod client to the @tanstack/solid-store reactive store.

The way he described it to me, I would have expected something similar to get it working properly in the Vue adapter: accessing the algodClient like this:

const algodClient = useStore(manager.store, (state) => state.algodClient)

and having that be what's returned from its useWallet composable function.

That's what I was planning to do in the Vue and React adapters when I created #213. Your changes here were also needed! I was just expecting the framework-specific reactive stores to also be part of the solution.

packages/use-wallet/src/manager.ts Outdated Show resolved Hide resolved
packages/use-wallet/src/manager.ts Outdated Show resolved Hide resolved
packages/use-wallet/src/manager.ts Outdated Show resolved Hide resolved
packages/use-wallet/src/store.ts Outdated Show resolved Hide resolved
@acfunk
Copy link
Contributor Author

acfunk commented Aug 30, 2024

That's what I was planning to do in the Vue and React adapters when I created #213. Your changes here were also needed! I was just expecting the framework-specific reactive stores to also be part of the solution.

Vue reactivity is handled in WalletManagerPlugin with const algodClient = ref(manager.algodClient).
I'll take a look at React.

EDIT: The React WalletProvider also already handles reactivity of algodClient.

Copy link
Collaborator

@drichar drichar left a comment

Choose a reason for hiding this comment

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

Looks great! LGTM

@drichar drichar changed the title move the algod client to store feat: move the algod client to store Sep 3, 2024
@drichar drichar merged commit 07e7273 into TxnLab:main Sep 3, 2024
1 check passed
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.

2 participants