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

fix(console): refactor useAllowance hook #336

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

baktun14
Copy link
Contributor

  • There was a circular reference between the WalletProvider and fee allowances creating a bug where the value of fee granter was undefined in the WalletProvider
  • Merged the useAllowance in the WalletProvider because they're intrinsically related and there's no more circular reference

@baktun14 baktun14 requested review from ygrishajev and Redm4x August 23, 2024 21:30
Copy link
Contributor

@ygrishajev ygrishajev left a comment

Choose a reason for hiding this comment

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

I understand the issue here but I can't say I like the solution. WalletProvider is already quite complex and large. I'd think on a better way to keep these two separate. In the worst case we can provide wallet as an argument to useAllowance.

@baktun14
Copy link
Contributor Author

baktun14 commented Aug 26, 2024

I understand the issue here but I can't say I like the solution. WalletProvider is already quite complex and large. I'd think on a better way to keep these two separate. In the worst case we can provide wallet as an argument to useAllowance.

I can try to refactor it back to being separated. I personally think it makes sense to have them in the same file, even though it's becoming a bit big.

@baktun14
Copy link
Contributor Author

@ygrishajev I refactored it back to 2 files but with params to the hook

@baktun14 baktun14 merged commit 269d227 into main Aug 26, 2024
5 checks passed
@baktun14 baktun14 deleted the bugfixes/console-fee-alowances branch August 26, 2024 19:18
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.

3 participants