Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Portal workspace to prefer the published @hemilabs/react-hooks and @hemilabs/wallet-watch-asset packages over locally maintained hook implementations and a GitHub-sourced dependency/patch, aligning the codebase with Issue #1807.
Changes:
- Add
@hemilabs/react-hooks@1.3.0and@hemilabs/wallet-watch-asset@1.0.2; remove the GitHubwallet-watch-assetdependency and its patch. - Replace several local hook implementations with re-exports from
@hemilabs/react-hooks. - Keep a local wrapper for
useEnsureConnectedToto extend the published EVM behavior with Bitcoin support.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| portal/package.json | Adds the published hook/watch-asset packages and removes the GitHub dependency. |
| portal/hooks/useWindowSize.ts | Re-exports useWindowSize from @hemilabs/react-hooks. |
| portal/hooks/useWatchedAsset.ts | Updates comment to reflect the published watch-asset storage key format. |
| portal/hooks/useOnKeyUp.ts | Re-exports useOnKeyUp from @hemilabs/react-hooks. |
| portal/hooks/useOnClickOutside.ts | Re-exports useOnClickOutside from @hemilabs/react-hooks. |
| portal/hooks/useInvalidateNativeBalanceAfterReceipt.ts | Re-exports useUpdateNativeBalanceAfterReceipt from @hemilabs/react-hooks. |
| portal/hooks/useEnsureConnectedTo.ts | Wraps published EVM ensure-connect hook and extends with Bitcoin network switching. |
| portal/hooks/useDebounce.ts | Re-exports useDebounce from @hemilabs/react-hooks. |
| portal/hooks/useBalance.ts | Uses published native balance hook while keeping local token balance logic. |
| portal/hooks/useAddTokenToWallet.ts | Switches import to @hemilabs/wallet-watch-asset. |
| patches/wallet-watch-asset+1.0.1.patch | Removes no-longer-needed patch for the old GitHub dependency. |
| package-lock.json | Updates lockfile to reflect the dependency swaps/additions/removals. |
You can also share your feedback on Copilot code review. Take the survey.
gndelia
left a comment
There was a problem hiding this comment.
I think the hooks should be consumed directly from the lib, and we should drop the files that only work as "intermediates" here if they don't add any value
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 54 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 88 out of 89 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
portal/hooks/useNeedsApprovalQuery.ts:32
useNeedsApprovalQuerycastsaddresstoAddressand callsuseAllowancefor all inputs. For native tokens in this codebase,token.addresscan be a non-0xsymbol (e.g.'ETH'), and this hook is invoked even in native-token flows (e.g. EVM deposit). This can lead to invalid contract reads and/or errors that incorrectly mark the approval state. Add anisNativeAddress/isAddressguard and ensure the allowance query is disabled andneedsApprovalisfalsefor native/invalid addresses, while still returning a safe query key value.
portal/utils/address.ts:7toChecksumAddressfalls back toaddress as AddresswhenisAddressis false. This defeats the purpose of the helper and can silently turn values like'ETH'(native token “address” in this repo) into anAddress, which then flows into viem/wagmi calls and can fail at runtime. Consider either throwing on invalid input or returningAddress | undefinedso call sites must explicitly handle non-0xvalues.
You can also share your feedback on Copilot code review. Take the survey.
portal/app/[locale]/stake/_components/manageStake/stakeOperation.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 88 out of 89 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
portal/hooks/useSaveTokenToWallet.ts:35
walletClient(andaddress) can beundefinedat runtime, but are passed directly towatchAsset.ensureConnectedToChain()may throw when there is no connected account, but it doesn't guaranteeuseWalletClienthas resolved a client for the target chain. Consider adding an explicit guard (and a clear error) whenwalletClientoraddressis missing before callingwatchAsset.
portal/hooks/useNeedsApprovalQuery.ts:32useAllowanceis called unconditionally withtoken.addresscast from astring. In this codebase, native tokens use non-0xaddresses (seeisNativeAddress), anduseNeedsApprovalQueryis invoked even for native tokens (e.g. EVM deposit). This can cause the allowance query to run with an invalid ERC20 address. Consider short-circuiting for native/invalid addresses (returnneedsApproval: falseand a safe query key), or passenabled: falseand a safe token address (e.g.zeroAddress) whenisNativeAddress(address)/!isAddress(address).
portal/utils/address.ts:7toChecksumAddressfalls back toaddress as Addresswhen the input is not a valid address. This makes the return type look safe (Address) while still allowing invalid values through to viem/wagmi, which can fail later in less obvious places. Consider throwing on invalid input, or returningAddress | undefinedso callers must handle invalid/non-EVM strings explicitly.
You can also share your feedback on Copilot code review. Take the survey.
portal/app/[locale]/stake/_components/manageStake/stakeOperation.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 88 out of 89 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
portal/hooks/useNeedsApprovalQuery.ts:25
useNeedsApprovalQuerycasts the incomingaddress: stringto aviemAddressand always callsuseAllowance. This hook is used with native tokens too (e.g. tunnel deposits), whereaddresscan bezeroAddressor even a non-0x symbol like"ETH"(seeisNativeAddress). That can lead to invalid contract reads / runtime errors and unnecessary queries. Consider guarding withisNativeAddress(address)/isAddress(address)and disabling/skipping the allowance query for native/invalid addresses (and returningneedsApproval: falseplus a safe placeholder query key).
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 88 out of 89 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
portal/hooks/useNeedsApprovalQuery.ts:32
useNeedsApprovalQuerycurrently castsaddresstoAddressand callsuseAllowanceunconditionally. This hook is invoked in places whereaddresscan be a native token identifier (e.g.ETH) or otherwise not a valid0x…ERC-20 address, which can cause runtime errors insideuseAllowance/contract reads and breaks the guarantee implied by theAddresstype. Add an explicit ERC-20 guard (e.g.isAddress(..., { strict:false })+!isNativeAddress(...)+!!owner) and pass anenabledflag (or skip) so the allowance query/key is only built for valid ERC-20 tokens; return a safe placeholder key for native/invalid tokens similar to thetokenBalanceQueryKeyadapter.
You can also share your feedback on Copilot code review. Take the survey.
gabmontes
left a comment
There was a problem hiding this comment.
The changes are unfortunately not trivial as initially expected. Even some hooks like useErc20TotalSupply were renamed adding up to the changes and complexity of the review.
Tests show something has broken within the app. I.e. the tunnel page is unable to enable the swap button, most probably due to an issue with the approval hooks.
Either retest the application to verify nothing was broken or split the PR into trivial hook replacements on one and all other hooks that needed to be adapted in another.
|
The issue is that the address is not validated before being passed to useAllowance cc: @gndelia |
gabmontes
left a comment
There was a problem hiding this comment.
Even when the PR seems to be ok now, merging it seems risky without extensive testing. There are too many changes and some were certainly not trivial.
I suggest splitting this PR into many smaller ones that only address one concern at a time:
- One PR that replaces the local wallet-watch-asset with the package and re-apply the patch.
- One PR that replaces all the hooks that are exact matches. It on changes references from local hooks to ones in the hooks package.
- One PR per hook that is not an exact match, including logic changes, adapting code, etc.
@gndelia do you think taking such an approach is an overkill and we should merge the PR as it is anyway? Any better approach?
|
I suggest spliting it into a few PRS. That approach of 3+ PRs looks good to me |

Description
Dependencies (portal): Add @hemilabs/react-hooks@1.3.0 and @hemilabs/wallet-watch-asset@1.0.2; remove wallet-watch-asset (GitHub) and delete patches/wallet-watch-asset+1.0.1.patch.
Hooks: Use or extend the published implementations: re-export from the package for useDebounce, useOnClickOutside, useOnKeyUp, useWindowSize, and useUpdateNativeBalanceAfterReceipt; use package useNativeBalance in useBalance.ts and keep local useTokenBalance; extend package useEnsureConnectedTo with Bitcoin support locally; switch useAddTokenToWallet to import from @hemilabs/wallet-watch-asset; update comment in useWatchedAsset.ts.
Implementation changes (logic modified or rewritten)
useNativeTokenBalancenow uses@hemilabs/react-hooks/useNativeBalance; local implementation removed;useTokenBalanceremains local.Re-exports only (no new local logic)
@hemilabs/react-hooks/useDebounce.@hemilabs/react-hooks/useOnClickOutside.@hemilabs/react-hooks/useOnKeyUp.@hemilabs/react-hooks/useWindowSize.useUpdateNativeBalanceAfterReceiptfrom@hemilabs/react-hooks/useUpdateNativeBalanceAfterReceipt.Other changes (import, comment, deps, patch)
wallet-watch-asset→@hemilabs/wallet-watch-asset.@hemilabs/react-hooksand@hemilabs/wallet-watch-asset; removedwallet-watch-asset.Summary: Two files have real implementation changes: useEnsureConnectedTo.ts and useBalance.ts. The rest are re-exports, import/comment updates, or dependency/config changes.
Screenshots
Related issue(s)
Closes #1807
Fixes #
Related to #
Checklist