feat: G$ Liquidity Adding Widget. (Ubeswap)#39
Open
kadiray34 wants to merge 1 commit intoGoodDollar:mainfrom
Open
feat: G$ Liquidity Adding Widget. (Ubeswap)#39kadiray34 wants to merge 1 commit intoGoodDollar:mainfrom
kadiray34 wants to merge 1 commit intoGoodDollar:mainfrom
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
getUserPositionsyou sequentially calltokenOfOwnerByIndexandpositionsfor each token; consider batching these withPromise.allor a multicall to avoid latency issues for users holding many positions. - Several calculations convert large
bigintvalues tonumber(e.g.Number(formatEther(...)),Number(liquidity),Number(this.sqrtPriceX96)); if positions or pool values grow this may introduce precision loss or overflow, so it may be worth constraining/validating ranges or using bigint-based math where possible. - The 30s
refreshIntervalinGooddollarLiquidityWidgetruns regardless of connection status and is not configurable; you might gate it onisConnectedor expose the interval as a prop to reduce unnecessary RPC traffic for integrators.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `getUserPositions` you sequentially call `tokenOfOwnerByIndex` and `positions` for each token; consider batching these with `Promise.all` or a multicall to avoid latency issues for users holding many positions.
- Several calculations convert large `bigint` values to `number` (e.g. `Number(formatEther(...))`, `Number(liquidity)`, `Number(this.sqrtPriceX96)`); if positions or pool values grow this may introduce precision loss or overflow, so it may be worth constraining/validating ranges or using bigint-based math where possible.
- The 30s `refreshInterval` in `GooddollarLiquidityWidget` runs regardless of connection status and is not configurable; you might gate it on `isConnected` or expose the interval as a prop to reduce unnecessary RPC traffic for integrators.
## Individual Comments
### Comment 1
<location path="packages/liquidity-widget/src/liquidity/tx-service.ts" line_range="20-23" />
<code_context>
+ account: `0x${string}`,
+ tokenAddress: `0x${string}`,
+ amount: bigint,
+ bufferPercent: number = 5,
+ callbacks?: TxCallbacks,
+): Promise<`0x${string}`> {
+ const bufferedAmount = amount + (amount * BigInt(bufferPercent) / 100n);
+
+ const { request } = await publicClient.simulateContract({
</code_context>
<issue_to_address>
**issue (bug_risk):** BigInt(bufferPercent) will throw for non-integer percentages, causing unexpected runtime errors.
Since `bufferPercent` is a `number`, callers can pass non-integers (e.g. 2.5), and `BigInt(bufferPercent)` will throw a `RangeError`. Either change the logic to support fractional values (e.g. scale to an integer before converting to `bigint` and use a matching divisor), or restrict/validate `bufferPercent` so only integers are allowed before calling `BigInt`.
</issue_to_address>
### Comment 2
<location path="packages/liquidity-widget/src/liquidity/tx-service.ts" line_range="87-96" />
<code_context>
+ return hash;
+}
+
+export async function removeLiquidity(
+ publicClient: PublicClient,
+ walletClient: WalletClient,
+ account: `0x${string}`,
+ tokenId: bigint,
+ liquidity: bigint,
+ callbacks?: TxCallbacks,
+): Promise<`0x${string}`> {
+ const deadline = BigInt(Math.floor(Date.now() / 1000) + 1200);
+
+ const { request: decreaseReq } = await publicClient.simulateContract({
+ account,
+ address: POSITION_MANAGER,
+ abi: POSITION_MANAGER_ABI,
+ functionName: 'decreaseLiquidity',
+ args: [{
+ tokenId,
+ liquidity: BigInt(liquidity) as unknown as bigint & { __brand: 'uint128' },
+ amount0Min: 0n,
+ amount1Min: 0n,
</code_context>
<issue_to_address>
**issue (bug_risk):** removeLiquidity does not enforce the uint128 limit, risking contract reverts for oversized liquidity values.
The `liquidity` value is cast/braned as `uint128` without validation, so callers can pass values > 2^128-1 and cause on-chain reverts. Since `MAX_UINT128` is already defined, add an explicit check (e.g. `liquidity <= MAX_UINT128`) or clamp before branding to keep the API safe and predictable.
</issue_to_address>
### Comment 3
<location path="packages/liquidity-widget/src/GooddollarLiquidityWidget.ts" line_range="533-536" />
<code_context>
+ this._validateInputs();
+ }
+
+ private _handleGdMax() {
+ this.gdInput = Number(formatEther(this.gdBalance)).toString();
+ this._calcUsdgloFromGd();
+ this.inputError = '';
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Converting formatted token balances through Number() can lose precision for large values.
Because this value is passed back into `parseEther`, the round‑trip via `Number` can produce a slightly different amount than the actual balance. Consider keeping the string returned by `formatEther`, or using a decimal-safe helper that avoids converting through `Number`.
```suggestion
private _handleGdMax() {
// Use the string returned by formatEther directly to avoid precision loss
this.gdInput = formatEther(this.gdBalance);
this._calcUsdgloFromGd();
this.inputError = '';
```
</issue_to_address>
### Comment 4
<location path="packages/liquidity-widget/README.md" line_range="7" />
<code_context>
+
+## Integrating The Component
+
+Can be used in any website, for a quick setup:
+
+1. **Download the Script**: Download the `index.global.js` file from the project releases or build it from the source.
</code_context>
<issue_to_address>
**suggestion (typo):** This sentence is missing a subject and has an unnecessary comma.
Consider: “It can be used on any website for a quick setup:” to add a clear subject and remove the unnecessary comma.
```suggestion
It can be used on any website for a quick setup:
```
</issue_to_address>
### Comment 5
<location path="packages/liquidity-widget/src/GooddollarLiquidityWidget.ts" line_range="474" />
<code_context>
+ if (isFinite(gd) && gd >= 0) this.gdInput = formatAmount(gd);
+ }
+
+ private _safeParseEther(value: string): bigint {
+ try {
+ if (!value || value.trim() === '' || value === '.') return 0n;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the input parsing/validation logic and repeated tx-step mutations into small shared helpers to keep the widget component focused on wiring state and UI only.
You can reduce complexity in this widget without changing behavior by extracting some of the heavier non‑UI logic into small helpers. Two low‑risk wins:
---
### 1. Isolate input parsing & validation
`_safeParseEther`, `_validateInputs`, and the input handlers all embed regex, parsing, and balance checks directly in the component. You can centralize this into a pure helper so the component only wires values and state.
**Before (inside component):**
```ts
private _safeParseEther(value: string): bigint {
try {
if (!value || value.trim() === '' || value === '.') return 0n;
return parseEther(value);
} catch { return 0n; }
}
private _validateInputs(force = false) {
this.inputError = '';
const re = /^[0-9]*\.?[0-9]*$/;
if (this.gdInput && !re.test(this.gdInput)) { this.inputError = 'Invalid G$ value'; return; }
if (this.usdgloInput && !re.test(this.usdgloInput)) { this.inputError = 'Invalid USDGLO value'; return; }
const gdNum = parseFloat(this.gdInput || '0');
const usdgloNum = parseFloat(this.usdgloInput || '0');
if (force && (isNaN(gdNum) || gdNum <= 0 || isNaN(usdgloNum) || usdgloNum <= 0)) {
this.inputError = 'Please enter valid amounts';
return;
}
if (gdNum > 0 && this._safeParseEther(this.gdInput) > this.gdBalance) {
this.inputError = 'Insufficient G$ balance';
return;
}
if (usdgloNum > 0 && this._safeParseEther(this.usdgloInput) > this.usdgloBalance) {
this.inputError = 'Insufficient USDGLO balance';
return;
}
}
```
**After (new `validation.ts` + slimmer component):**
```ts
// validation.ts
import { parseEther } from 'viem';
export function safeParseEther(value: string): bigint {
try {
if (!value || value.trim() === '' || value === '.') return 0n;
return parseEther(value);
} catch {
return 0n;
}
}
const INPUT_RE = /^[0-9]*\.?[0-9]*$/;
export function validateInputs(params: {
gdInput: string;
usdgloInput: string;
gdBalance: bigint;
usdgloBalance: bigint;
force?: boolean;
}): string | null {
const { gdInput, usdgloInput, gdBalance, usdgloBalance, force } = params;
if (gdInput && !INPUT_RE.test(gdInput)) return 'Invalid G$ value';
if (usdgloInput && !INPUT_RE.test(usdgloInput)) return 'Invalid USDGLO value';
const gdNum = parseFloat(gdInput || '0');
const usdgloNum = parseFloat(usdgloInput || '0');
if (force && (isNaN(gdNum) || gdNum <= 0 || isNaN(usdgloNum) || usdgloNum <= 0)) {
return 'Please enter valid amounts';
}
if (gdNum > 0 && safeParseEther(gdInput) > gdBalance) return 'Insufficient G$ balance';
if (usdgloNum > 0 && safeParseEther(usdgloInput) > usdgloBalance) return 'Insufficient USDGLO balance';
return null;
}
```
```ts
// in component
import { safeParseEther, validateInputs } from './liquidity/validation';
private _validateInputs(force = false) {
this.inputError = validateInputs({
gdInput: this.gdInput,
usdgloInput: this.usdgloInput,
gdBalance: this.gdBalance,
usdgloBalance: this.usdgloBalance,
force,
}) ?? '';
}
private _getButtonLabel(): string {
if (this.txPhase === 'success') return 'Add More Liquidity';
const gdWei = safeParseEther(this.gdInput);
const usdgloWei = safeParseEther(this.usdgloInput);
if (gdWei > 0n && this.gdAllowance < gdWei) return 'Approve & Add Liquidity';
if (usdgloWei > 0n && this.usdgloAllowance < usdgloWei) return 'Approve & Add Liquidity';
return 'Add Liquidity';
}
```
This makes handlers like `_handleGdInput/_handleUsdgloInput` just update strings and call the pure validator.
---
### 2. Encapsulate repeated step mutations in the tx flow
`_handleMainAction` mutates `steps` in place several times:
```ts
steps[0].status = 'active'; this.txSteps = [...steps];
...
steps[0].status = 'completed'; steps[0].txHash = hash; this.txSteps = [...steps];
...
steps[1].status = 'active'; this.txSteps = [...steps];
...
steps[1].status = 'completed'; steps[1].txHash = hash; this.txSteps = [...steps];
...
steps[2].status = 'active'; this.txSteps = [...steps];
...
steps[2].status = 'completed'; steps[2].txHash = hash; this.txSteps = [...steps];
```
You can keep the entire flow in the component but extract a tiny helper to update step status + tx hash, reducing repetition and making it easier to change step shape later.
```ts
// tx-steps.ts
import type { TxStepInfo } from './liquidity/types';
export function updateStep(
steps: TxStepInfo[],
index: number,
patch: Partial<TxStepInfo>,
): TxStepInfo[] {
const next = steps.slice();
next[index] = { ...next[index], ...patch };
return next;
}
```
```ts
// in component
import { updateStep } from './liquidity/tx-steps';
async _handleMainAction() {
// ... existing setup ...
const steps: TxStepInfo[] = [
{ label: 'Approve G$', status: needGdApproval ? 'pending' : 'skipped' },
{ label: 'Approve USDGLO', status: needUsdgloApproval ? 'pending' : 'skipped' },
{ label: 'Add Liquidity', status: 'pending' },
];
this.txSteps = steps;
this.txError = '';
try {
if (needGdApproval) {
this.txPhase = 'approving-gd';
this.txHash = '';
this.txSteps = updateStep(steps, 0, { status: 'active' });
const hash = await approveToken(/* ... */);
this.txSteps = steps = updateStep(steps, 0, { status: 'completed', txHash: hash });
await this.refreshData();
}
if (needUsdgloApproval) {
this.txPhase = 'approving-usdglo';
this.txHash = '';
this.txSteps = updateStep(steps, 1, { status: 'active' });
const hash = await approveToken(/* ... */);
this.txSteps = steps = updateStep(steps, 1, { status: 'completed', txHash: hash });
await this.refreshData();
}
this.txPhase = 'minting';
this.txHash = '';
this.txSteps = updateStep(steps, 2, { status: 'active' });
const hash = await addLiquidity(/* ... */);
this.txSteps = updateStep(steps, 2, { status: 'completed', txHash: hash });
// ... rest unchanged ...
} catch (error: any) {
// unchanged
}
}
```
This keeps the same behavior and UX while making `_handleMainAction` shorter, more regular, and easier to extend (e.g., new steps) without duplicating mutation patterns.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+20
to
+23
| bufferPercent: number = 5, | ||
| callbacks?: TxCallbacks, | ||
| ): Promise<`0x${string}`> { | ||
| const bufferedAmount = amount + (amount * BigInt(bufferPercent) / 100n); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): BigInt(bufferPercent) will throw for non-integer percentages, causing unexpected runtime errors.
Since bufferPercent is a number, callers can pass non-integers (e.g. 2.5), and BigInt(bufferPercent) will throw a RangeError. Either change the logic to support fractional values (e.g. scale to an integer before converting to bigint and use a matching divisor), or restrict/validate bufferPercent so only integers are allowed before calling BigInt.
Comment on lines
+87
to
+96
| export async function removeLiquidity( | ||
| publicClient: PublicClient, | ||
| walletClient: WalletClient, | ||
| account: `0x${string}`, | ||
| tokenId: bigint, | ||
| liquidity: bigint, | ||
| callbacks?: TxCallbacks, | ||
| ): Promise<`0x${string}`> { | ||
| const deadline = BigInt(Math.floor(Date.now() / 1000) + 1200); | ||
|
|
Contributor
There was a problem hiding this comment.
issue (bug_risk): removeLiquidity does not enforce the uint128 limit, risking contract reverts for oversized liquidity values.
The liquidity value is cast/braned as uint128 without validation, so callers can pass values > 2^128-1 and cause on-chain reverts. Since MAX_UINT128 is already defined, add an explicit check (e.g. liquidity <= MAX_UINT128) or clamp before branding to keep the API safe and predictable.
Comment on lines
+533
to
+536
| private _handleGdMax() { | ||
| this.gdInput = Number(formatEther(this.gdBalance)).toString(); | ||
| this._calcUsdgloFromGd(); | ||
| this.inputError = ''; |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Converting formatted token balances through Number() can lose precision for large values.
Because this value is passed back into parseEther, the round‑trip via Number can produce a slightly different amount than the actual balance. Consider keeping the string returned by formatEther, or using a decimal-safe helper that avoids converting through Number.
Suggested change
| private _handleGdMax() { | |
| this.gdInput = Number(formatEther(this.gdBalance)).toString(); | |
| this._calcUsdgloFromGd(); | |
| this.inputError = ''; | |
| private _handleGdMax() { | |
| // Use the string returned by formatEther directly to avoid precision loss | |
| this.gdInput = formatEther(this.gdBalance); | |
| this._calcUsdgloFromGd(); | |
| this.inputError = ''; |
Collaborator
|
@kadiray34 can you make a feature-branch in your work and create the pull-request against that branch instead of working on main |
L03TJ3
requested changes
Apr 6, 2026
Comment on lines
+25
to
+31
| export const POOL_ABI = parseAbi([ | ||
| 'function slot0() view returns (uint160 sqrtPriceX96, int24 tick, uint16 observationIndex, uint16 observationCardinality, uint16 observationCardinalityNext, uint8 feeProtocol, bool unlocked)', | ||
| ]); | ||
|
|
||
| export const POSITION_MANAGER_ABI = [ | ||
| { | ||
| inputs: [ |
Collaborator
There was a problem hiding this comment.
Why choose to mix ABI definitions?
parseAbi is more readable from my pov + is used in almost any other sdk (including savings sdk)
| ) { | ||
| const [gdBalance, usdgloBalance, gdAllowance, usdgloAllowance] = await Promise.all([ | ||
| publicClient.readContract({ | ||
| address: GD_TOKEN, abi: (await import('./constants')).ERC20_ABI, |
Collaborator
There was a problem hiding this comment.
-
no need for dynamic import, can just import ERC20_ABI.
-
can use publicClient.multicall
const [gdBalance, usdgloBalance, gdAllowance, usdgloAllowance] =
await publicClient.multicall({
contracts: [
{ address: GD_TOKEN, abi: ERC20_ABI, functionName: 'balanceOf', args: [userAddress] },
{ address: USDGLO_TOKEN, abi: ERC20_ABI, functionName: 'balanceOf', args: [userAddress] },
{ address: GD_TOKEN, abi: ERC20_ABI, functionName: 'allowance', args: [userAddress, POSITION_MANAGER] },
{ address: USDGLO_TOKEN, abi: ERC20_ABI, functionName: 'allowance', args: [userAddress, POSITION_MANAGER] },
],
allowFailure: false,
});
| return hash; | ||
| } | ||
|
|
||
| export function parseTxError(error: any): string { |
Collaborator
There was a problem hiding this comment.
for easier extending possible errors to support, consider using a error message map instead of repeating this logic
| splitting: false, | ||
| sourcemap: false, | ||
| clean: true, | ||
| dts: false, |
Collaborator
There was a problem hiding this comment.
the webcomponent might be released as npm package for environments that support it.
considering react/ts environments dts should be turned on
| this.txSteps = [...steps]; | ||
| this.txError = ''; | ||
|
|
||
| try { |
Collaborator
There was a problem hiding this comment.
It seems the steps are not applying their statusses as expected/intended.
not sure if its fix here or in lw-stepper
UI:
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This widget is for adding G$-USDGLO liquidity on Ubeswap. Users can add liquidity and manage through this widget.
Summary by Sourcery
Introduce a reusable Gooddollar liquidity web component for adding and tracking G$/USDGLO liquidity on Ubeswap, including on-chain interactions, range presets, and build configuration.
New Features:
Enhancements:
Build:
Documentation: