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

chore: Prevent Undefined address in useWalletClient Calls #243

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

famouswizard
Copy link

@famouswizard famouswizard commented Feb 16, 2025

I fixed a potential issue where address could be undefined when passed to useWalletClient.

Now, before using address, the code explicitly checks its existence:

  • If address is missing, an error is thrown: 'No address found for the global wallet signer.'
  • This prevents unintended behavior and improves error handling.

PR-Codex overview

This PR enhances the useGlobalWalletSignerClient function by adding error handling to ensure that a valid wallet address is available before proceeding with the wallet client setup.

Detailed summary

  • Added a check for the address variable.
  • If address is not found, an error is thrown with the message: 'No address found for the global wallet signer.'

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link
Collaborator

@coffeexcoin coffeexcoin left a comment

Choose a reason for hiding this comment

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

I don't think throwing here is the right approach - are you running into a scenario where this is undefined?

If you are, we might need to update this to wait on running the subsequent logic until the useGlobalWalletSignerAccount query is resolved.

@famouswizard
Copy link
Author

I don't think throwing here is the right approach - are you running into a scenario where this is undefined?

If you are, we might need to update this to wait on running the subsequent logic until the useGlobalWalletSignerAccount query is resolved.

I actually did run into a case where address was undefined.
That’s why I added this check—to prevent potential issues when the value isn’t available.

That said, if waiting for useGlobalWalletSignerAccount to resolve is the preferred approach, I’m happy to update the implementation accordingly.

@coffeexcoin
Copy link
Collaborator

I don't think throwing here is the right approach - are you running into a scenario where this is undefined?
If you are, we might need to update this to wait on running the subsequent logic until the useGlobalWalletSignerAccount query is resolved.

I actually did run into a case where address was undefined. That’s why I added this check—to prevent potential issues when the value isn’t available.

That said, if waiting for useGlobalWalletSignerAccount to resolve is the preferred approach, I’m happy to update the implementation accordingly.

Yes, lets update it so the useGlobalWalletSignerClient query doesn't resolve unless the useGlobalWalletSignerAccount is fully resolved, I suspect this should resolve what appears to just be a race condition on the hook return

@famouswizard
Copy link
Author

I don't think throwing here is the right approach - are you running into a scenario where this is undefined?
If you are, we might need to update this to wait on running the subsequent logic until the useGlobalWalletSignerAccount query is resolved.

I actually did run into a case where address was undefined. That’s why I added this check—to prevent potential issues when the value isn’t available.
That said, if waiting for useGlobalWalletSignerAccount to resolve is the preferred approach, I’m happy to update the implementation accordingly.

Yes, lets update it so the useGlobalWalletSignerClient query doesn't resolve unless the useGlobalWalletSignerAccount is fully resolved, I suspect this should resolve what appears to just be a race condition on the hook return

Lets fix it.

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