Feat Integrate Ubeswaps saving widget.#634
Conversation
Feat: Integrate ubeswap savings widget GoodDollar#619
…oodDollar#626) * fix: replace useModal with absolute overlay for disabled claim state * fix: gate modal display with claimEnabled flag and remove comments * fix: use standard img tag to ensure maintenance asset renders * chore: restore valid developer comments and remove temporary ones
…d approval amount
* fix: stop forcing celo gas price * chore: bump gooddollar sdks --------- Co-authored-by: LewisB <lewis@ikigaistudios.eu>
* add GoodBridge page to src/pages/gd * update sideBar.tsx * feat: implement GoodBridge route and update connect wallet copy * Delete src/pages/gd/GoodBridge/index.tsx~ * Add goodBridgeEnabled to payload destructuring * Fix typo in goodBridgeEnabled variable * Update bridge names in SideBar component
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the new Savings page,
widgetRefis typed asanyand you’re assigningconnectWallet/web3Providerdynamically on the custom element; consider defining a dedicated TypeScript interface for the widget element (or extendingHTMLElementTagNameMap) and using a typeduseRefto make these assignments type-safe and to catch property name mismatches at compile time. - The new
decodeTransactionErrorReasonhelper assumes string-like error shapes but is called both witherrorMessagestrings and raw error objects; it may be safer to normalize common error types (e.g.,Error,EthersErrorwithdatapayload) explicitly before regex parsing so that unexpected shapes don’t end up as confusing generic messages.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new Savings page, `widgetRef` is typed as `any` and you’re assigning `connectWallet`/`web3Provider` dynamically on the custom element; consider defining a dedicated TypeScript interface for the widget element (or extending `HTMLElementTagNameMap`) and using a typed `useRef` to make these assignments type-safe and to catch property name mismatches at compile time.
- The new `decodeTransactionErrorReason` helper assumes string-like error shapes but is called both with `errorMessage` strings and raw error objects; it may be safer to normalize common error types (e.g., `Error`, `EthersError` with `data` payload) explicitly before regex parsing so that unexpected shapes don’t end up as confusing generic messages.
## Individual Comments
### Comment 1
<location path="src/hooks/useWeb3.tsx" line_range="231-235" />
<code_context>
+ []
+ )
+
+ const filterBlockedRpcs = (rpcs: Record<string, string[]>) =>
+ Object.fromEntries(
+ Object.entries(rpcs).map(([chainId, urls]) => [
+ chainId,
+ urls.filter((url) => !excludedRpcs.some((blocked) => url.toLowerCase().includes(blocked))),
+ ])
+ )
</code_context>
<issue_to_address>
**issue:** Handle cases where all RPC URLs for a chain are filtered out by excludedRpcs.
If `excludedRpcs` matches all URLs for a chain, `filterBlockedRpcs` will return an empty array for that chain, and any code assuming at least one URL (e.g. `testedRpcs[chainId][0]`) may fail at runtime.
Consider either:
- falling back to the original URLs when filtering yields an empty array, or
- omitting those chains entirely and ensuring the rest of the app tolerates missing `testedRpcs[chainId]`.
This avoids hard failures when `REACT_APP_EXCLUDED_RPCS` is misconfigured.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const filterBlockedRpcs = (rpcs: Record<string, string[]>) => | ||
| Object.fromEntries( | ||
| Object.entries(rpcs).map(([chainId, urls]) => [ | ||
| chainId, | ||
| urls.filter((url) => !excludedRpcs.some((blocked) => url.toLowerCase().includes(blocked))), |
There was a problem hiding this comment.
issue: Handle cases where all RPC URLs for a chain are filtered out by excludedRpcs.
If excludedRpcs matches all URLs for a chain, filterBlockedRpcs will return an empty array for that chain, and any code assuming at least one URL (e.g. testedRpcs[chainId][0]) may fail at runtime.
Consider either:
- falling back to the original URLs when filtering yields an empty array, or
- omitting those chains entirely and ensuring the rest of the app tolerates missing
testedRpcs[chainId].
This avoids hard failures when REACT_APP_EXCLUDED_RPCS is misconfigured.
|
@Oluwatomilola Still finding it difficult to resolve the conflicts? |
…t-integrat # Conflicts: # package.json # yarn.lock
|
@pheobeayo |
pheobeayo
left a comment
pheobeayo
left a comment
There was a problem hiding this comment.
Good progress — the npm import is correctly in place, loadWidgetScript is gone, the FAQ has been expanded, and the duplicate type file is resolved. The main blockers from #625 are addressed.
A few things still need attention:
1. ref prop missing from type declaration (types/gooddollar-savings-widget.d.ts)
The current declaration doesn't include ref, which means TypeScript won't properly type the widgetRef assignment. Update the intrinsic element declaration to:
'gooddollar-savings-widget': React.DetailedHTMLProps<React.HTMLAttributes<HTMLElement>, HTMLElement> & {
ref?: React.Ref<HTMLElement & { connectWallet?: () => void; web3Provider?: unknown | null }>
}2. useRef<any> — avoid any
Use a proper type consistent with the declaration above:
const widgetRef = useRef<HTMLElement & {
connectWallet?: () => void
web3Provider?: unknown | null
}>(null)3. Merged useEffect causes unnecessary re-assignment of connectWallet
The first useEffect now assigns connectWallet on every change to walletProvider or address, which is unnecessary. Split them back into two separate effects:
useEffect(() => {
const widget = widgetRef.current
if (!widget) return
widget.connectWallet = handleConnectWallet
}, [handleConnectWallet])
useEffect(() => {
const widget = widgetRef.current
if (!widget) return
widget.web3Provider = (address && walletProvider) ? walletProvider : null
}, [address, walletProvider])With this, the second disconnect useEffect can be removed entirely.
4. Verify walletProvider type compatibility with the widget
The switch from useWeb3Context (@gooddollar/web3sdk-v2) to useAppKitProvider<Provider>('eip155') changes what gets passed as web3Provider. Please confirm that the gooddollar-savings-widget accepts a raw AppKit Provider and not specifically an ethers Web3Provider otherwise this will fail silently at runtime.
Once these are addressed this should be good to merge.
github-project-automation
Bot
moved this from Ready-For-Assignment
to In Progress
in GoodBounties
L03TJ3
left a comment
L03TJ3
left a comment
There was a problem hiding this comment.
Looks good, just included a suggestion so that we can more cleanly test it on our development environment.
Not an action item! I can commit through the PR
|
okay, it look good on the surface but after QA'in there are issues with the widget. can you explain if and how you tested this? have you actually deposited something into savings using the widget? |
|
Yes, I tested the widget end-to-end locally using my connected MetaMask wallet with Celo. Test flow: Then I verified that transactions completed successfully in the UI and on-chain via Celoscan Transaction proofs: From my testing, the integration itself is functioning correctly in the frontend. The issues mentioned during QA appear to originate from the underlying SDK/widget behavior rather than this PR integration layer. |
|
The question I have is: why not tell or raise issues you encountered? Silently ignoring weird things/bugs.. they will never be handled or solved. The issues were quite clear when it came to UX: technical error stacks in ui, approve > stake flow throwing errors. That is bugs. even if the core functionality works, its UX bugs. What I encountered is described here: GoodDollar/GoodSDKs#46 |
github-project-automation
Bot
moved this from In Review
to Deploy and Verify
in GoodBounties
|
My bad : ( I was focused primarily on validating the integration layer and core transaction flow, so once staking and unstaking succeeded on-chain I treated the PR as functionally complete. I should have explicitly documented the UX/runtime issues I encountered during testing instead of assuming they were already known SDK limitations. I mistakenly separated those from the scope of the PR because the underlying transactions still executed successfully, but I agree those are still user-facing bugs and should have been raised clearly for visibility and follow-up. Going forward I'll make sure to document all observed issues during QA — even when they originate from upstream SDK behavior — so they can be tracked, discussed, and resolved instead of silently passing through review. Thanks for calling that out. |
|
We are going forward with a new bounty/contributions structure. Since you were assigned before the new structure make sure you follow the rewards as defined here: https://github.com/GoodDollar/.github/blob/1479d6c5350ea063788eb8052407b6a6054b3c8a/CONTRIBUTING.md#bounty-tiers-explained I mention the new structure because it's also your first time contributing and having a PR merged. Make sure you use a GoodDollar verified wallet! |
|
Acknowledged. |
|
@L03TJ3 |
7 participants
Description
This PR introduces the GoodDollar Savings Widget integration into the GoodDollar UI, allowing users to access the savings/staking interface directly within the app.
The implementation follows the recommended approach using the published package (
@goodsdks/savings-widget) instead of script injection, ensuring a production-safe and maintainable integration.Key Changes
Added a new **Savings page that renders the
<gooddollar-savings-widget />Integrated wallet connection using existing hooks (
useAppKit,useAppKitAccount,useWeb3Context)Passed
web3Providerto the widget after wallet connectionAdded Savings FAQ entries for better user context (rewards, network, withdrawals)
Extended
FaqTypeto include"savings"Added global TypeScript declaration for the custom web component
Ensured minimal and scoped changes aligned with contribution guidelines
Stability Fixes (Local Dev)
Added defensive guards to prevent crashes when analytics/feature flags (PostHog) are unavailable in local environments
Ensures the app renders without runtime errors during development
Dependencies
@goodsdks/savings-widgetFixes #619
Closes #625
How Has This Been Tested?
Ran the app locally using
yarn startNavigated to
/savingsand verified:Widget renders correctly
No blank screen or runtime crashes
Tested wallet connection flow:
"Connect Wallet” triggers AppKit modal
web3Provideris correctly passed to the widget after connectionProvider resets on disconnect
Verified:
No console errors related to the widget
No TypeScript errors (
yarn tsc --noEmit)No lint errors
Confirmed no regressions in other parts of the app
Checklist:
@pheobeayo
Screenshots / Demo
Summary by Sourcery
Integrate a new savings experience and bridge flow into the GoodDollar app while improving wallet connection behavior, RPC handling, and error reporting.
New Features:
Bug Fixes:
Enhancements:
Build: