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

bugfix: init algod client #221

Merged
merged 3 commits into from
Aug 29, 2024
Merged

bugfix: init algod client #221

merged 3 commits into from
Aug 29, 2024

Conversation

acfunk
Copy link
Contributor

@acfunk acfunk commented Aug 23, 2024

The algod client should be created from the stored network if it exists, not always from the parameter passed in.

@acfunk
Copy link
Contributor Author

acfunk commented Aug 24, 2024

This should now also fix #213

@drichar
Copy link
Collaborator

drichar commented Aug 29, 2024

Thank you for the PR @acfunk! The initial bug fix, using the active network from initialState instead of the config param, looks good to merge.

In your proposed fix for #213 though, there are now multiple algod client instances being created and stored in multiple places inside WalletManager. If the goal is to move the client to the store, this.algodClient in the manager should probably be replaced by a derived property (getter method) that returns that one (only) instance from the store. The createAlgodClient method should not need to be called twice in the constructor.

Instead of trying to fix two issues in a single pull request, I'd suggest moving the fix for #213 into a separate one. That way I can merge the first fix right away and we can discuss the other changes separately.

@acfunk
Copy link
Contributor Author

acfunk commented Aug 29, 2024

Sounds good. I have reverted the commit aimed at #213, leaving only the one-liner fix for the initial state.

@drichar drichar merged commit 2ff796b into TxnLab:main Aug 29, 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