feat: G$ Liquidity Adding Widget. (Ubeswap)#41
Conversation
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
pool-service.ts, several calculations convert large on-chain values tonumber(e.g.sqrtPriceX96inloadPoolData,liquidityincalculatePositionAmounts, andformatBigIntDisplayusingNumber(formatEther(num))), which can overflow or lose precision for realistic uint160/uint128 ranges; consider reusing the bigint/fixed-point helpers fromGooddollarLiquidityWidgetor adding shared safe math utilities to avoid precision bugs. loadPoolDatacomputesprice/gdPriceInUsdglousing a different, less precise path than_getGdPriceInUsdgloinGooddollarLiquidityWidget; to keep behavior consistent and reduce duplicated logic, consider centralizing the price computation in a shared helper and reusing it in both places.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `pool-service.ts`, several calculations convert large on-chain values to `number` (e.g. `sqrtPriceX96` in `loadPoolData`, `liquidity` in `calculatePositionAmounts`, and `formatBigIntDisplay` using `Number(formatEther(num))`), which can overflow or lose precision for realistic uint160/uint128 ranges; consider reusing the bigint/fixed-point helpers from `GooddollarLiquidityWidget` or adding shared safe math utilities to avoid precision bugs.
- `loadPoolData` computes `price`/`gdPriceInUsdglo` using a different, less precise path than `_getGdPriceInUsdglo` in `GooddollarLiquidityWidget`; to keep behavior consistent and reduce duplicated logic, consider centralizing the price computation in a shared helper and reusing it in both places.
## Individual Comments
### Comment 1
<location path="packages/liquidity-widget/src/liquidity/pool-service.ts" line_range="148" />
<code_context>
+ const sqrtPriceUpper = tickToSqrtPrice(tickUpper);
+ const sqrtPriceCurrent = tickToSqrtPrice(currentTick);
+
+ const L = Number(liquidity);
+ let amount0 = 0;
+ let amount1 = 0;
</code_context>
<issue_to_address>
**issue (bug_risk):** Converting `liquidity` from bigint to number risks overflow and precision loss for large positions.
`liquidity` is a uint128 and may exceed JS’s safe integer range. Converting it to `number` can silently lose precision or overflow to `Infinity`, which will corrupt `amount0`/`amount1`. Keep these calculations in `bigint` (e.g., with fixed-point scaling), or at minimum validate/clamp the value and throw if it cannot be represented safely as a `number`.
</issue_to_address>
### Comment 2
<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):** Consider rephrasing this fragment into a complete sentence for clarity.
For example: “It can be used on any website. For a quick setup:” or “This component can be used on any website; for a quick setup:” to avoid the fragment and the awkward comma.
```suggestion
This component can be used on any website. For a quick setup:
```
</issue_to_address>
### Comment 3
<location path="packages/liquidity-widget/README.md" line_range="63-64" />
<code_context>
+- **`connectWallet`**: _(Set via JavaScript property)_
+ Defines the function called when the "Connect Wallet" button is clicked.
+
+- **`web3Provider`**: _(Set via JavaScript property)_
+ The web3Provider object when the wallet is connected. Wallet connection logic should be handled outside of this component.
+
+- **`explorerBaseUrl`**: _(String, default: `"https://celoscan.io"`)_
</code_context>
<issue_to_address>
**suggestion (typo):** The `web3Provider` description is missing a verb, which makes the sentence slightly unclear.
Consider rephrasing to: "The web3Provider object to use when the wallet is connected." while keeping the rest of the description unchanged.
```suggestion
- **`web3Provider`**: _(Set via JavaScript property)_
The web3Provider object to use when the wallet is connected. Wallet connection logic should be handled outside of this component.
```
</issue_to_address>
### Comment 4
<location path="packages/liquidity-widget/src/GooddollarLiquidityWidget.ts" line_range="99" />
<code_context>
+ private userAddress: string | null = null;
+ private _refreshTimer: ReturnType<typeof setInterval> | null = null;
+
+ // ── Bigint helpers (avoid precision loss) ──────────────────────────
+
+ private static readonly Q96 = 2n ** 96n;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the price math/bigint helpers and the transaction step-building logic into shared helpers/services so the widget focuses on wiring data and UI only.
You can trim quite a bit of complexity here by centralizing the math and step orchestration into helpers. Two concrete refactors that keep behavior the same:
---
### 1. Remove custom bigint/fixed‑point helpers from the widget
Right now the widget re‑derives price from `sqrtPriceX96` using `_pow10`, `_formatFixed`, `_toFiniteNumber`, `Q96`, `Q192`, `_getGdPriceInUsdglo`, `_getSqrtPriceFloat`. That logic is only used for:
- Displaying `gdPrice` in `<lw-pool-info>`
- Range/amount pairing (`_calcAmount1From0`, `_calcAmount0From1`)
`loadPoolData` already has all the raw data and can be the single source of truth for human‑scale numbers.
**Actionable change:**
1. Extend `loadPoolData` to compute the float values once:
```ts
// pool-service.ts
export type PoolData = {
sqrtPriceX96: bigint;
currentTick: number;
gdPriceInUsdglo: number;
sqrtPriceFloat: number; // for range pairing
};
export async function loadPoolData(client: PublicClient): Promise<PoolData> {
// ... existing on-chain reads
const sqrtPriceX96 = /* ... */;
const currentTick = /* ... */;
const gdPriceInUsdglo = computeGdPriceFromSqrtPrice(sqrtPriceX96);
const sqrtPriceFloat = computeSqrtPriceFloat(sqrtPriceX96);
return { sqrtPriceX96, currentTick, gdPriceInUsdglo, sqrtPriceFloat };
}
```
2. Store `poolData` on the widget and use it directly:
```ts
// widget state
@state() private poolData: PoolData | null = null;
@state() private currentTick = 0;
// in refreshData()
const pool = await loadPoolData(this.publicClient);
this.poolData = pool;
this.currentTick = pool.currentTick;
// in render()
const gdPrice = this.poolData?.gdPriceInUsdglo ?? 0;
```
3. Use `sqrtPriceFloat` for range pairing, and delete the bigint helpers and the `_getGdPriceInUsdglo` / `_getSqrtPriceFloat` methods:
```ts
private _getSqrtPriceFloat(): number {
return this.poolData?.sqrtPriceFloat ?? 0;
}
```
This lets you remove:
- `Q96`, `Q192`
- `_pow10`, `_formatFixed`, `_toFiniteNumber`
- `_getGdPriceInUsdglo`
while keeping the same UI behavior and range math.
---
### 2. Extract tx step building/orchestration
`_handleMainAction` currently:
- Validates inputs
- Computes needed approvals
- Builds steps
- Mutates `steps` and `this.txSteps` repeatedly
- Handles three phases (2 approvals + mint) inline
You can push step construction and orchestration into small helpers so the component only wires them.
**Actionable change:**
1. Extract step construction to a pure helper:
```ts
// tx-steps.ts (or same file above the class)
export function buildTxSteps(params: {
gdWei: bigint;
usdgloWei: bigint;
gdAllowance: bigint;
usdgloAllowance: bigint;
}): { steps: TxStepInfo[]; needGdApproval: boolean; needUsdgloApproval: boolean } {
const { gdWei, usdgloWei, gdAllowance, usdgloAllowance } = params;
const needGdApproval = gdWei > 0n && gdAllowance < gdWei;
const needUsdgloApproval = usdgloWei > 0n && usdgloAllowance < usdgloWei;
const steps: TxStepInfo[] = [
{ label: 'Approve G$', status: needGdApproval ? 'pending' : 'skipped' },
{ label: 'Approve USDGLO', status: needUsdgloApproval ? 'pending' : 'skipped' },
{ label: 'Add Liquidity', status: 'pending' },
];
return { steps, needGdApproval, needUsdgloApproval };
}
```
2. Use that in `_handleMainAction` to simplify the top half of the method:
```ts
async _handleMainAction() {
if (!this.walletClient || !this.publicClient || !this.userAddress) return;
if (this.txPhase === 'success') {
this.txPhase = 'idle';
this.txHash = '';
this.txSteps = [];
return;
}
this._validateInputs(true);
if (this.inputError) return;
const account = this.userAddress as `0x${string}`;
const gdWei = safeParseEther(this.gdInput);
const usdgloWei = safeParseEther(this.usdgloInput);
const { steps, needGdApproval, needUsdgloApproval } = buildTxSteps({
gdWei,
usdgloWei,
gdAllowance: this.gdAllowance,
usdgloAllowance: this.usdgloAllowance,
});
this.txSteps = steps;
this.txError = '';
try {
// existing approval + addLiquidity flow stays the same,
// but now uses needGdApproval / needUsdgloApproval and updateStep(steps, ...)
} catch (error: any) {
this.txPhase = 'error';
this.txError = parseTxError(error);
this._emitTxEvent('lw-tx-failed', this.txHash, this.txPhase, error);
}
}
```
You keep `updateStep` as-is, but `_handleMainAction` becomes shorter and less intertwined with the step decision logic. If you want to go further, you can also extract the approval+mint sequence into a small `runAddLiquidityFlow(...)` helper that receives callbacks, but even just factoring `buildTxSteps` already reduces the cognitive load in the component.
</issue_to_address>
### Comment 5
<location path="packages/liquidity-widget/src/liquidity/pool-service.ts" line_range="136" />
<code_context>
+ return positions;
+}
+
+function calculatePositionAmounts(
+ liquidity: bigint,
+ tickLower: number,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting `calculatePositionAmounts` into a shared, well‑named utility that encapsulates the tick/price branching logic and bigint-to-number approximation.
The `calculatePositionAmounts` helper is doing correct Uniswap-style math, but the bigint → float → bigint conversions and inlined branching make it harder to reason about and duplicate logic elsewhere.
You can reduce complexity and duplication by:
1. Extracting the math into a shared utility with clear naming.
2. Making the numeric domain explicit (UI-only approximation vs exact on-chain style).
For example, move the logic into something like `liquidityMath.ts`:
```ts
// liquidityMath.ts
import { tickToSqrtPrice } from './constants';
export function getAmountsForPositionApprox(
liquidity: bigint,
tickLower: number,
tickUpper: number,
currentTick: number,
): { amount0: bigint; amount1: bigint } {
if (liquidity === 0n) return { amount0: 0n, amount1: 0n };
const sqrtPriceLower = tickToSqrtPrice(tickLower);
const sqrtPriceUpper = tickToSqrtPrice(tickUpper);
const sqrtPriceCurrent = tickToSqrtPrice(currentTick);
const L = Number(liquidity); // explicitly approximate
let amount0 = 0;
let amount1 = 0;
if (currentTick < tickLower) {
amount0 = L * (sqrtPriceUpper - sqrtPriceLower) / (sqrtPriceLower * sqrtPriceUpper);
} else if (currentTick >= tickUpper) {
amount1 = L * (sqrtPriceUpper - sqrtPriceLower);
} else {
amount0 = L * (sqrtPriceUpper - sqrtPriceCurrent) / (sqrtPriceCurrent * sqrtPriceUpper);
amount1 = L * (sqrtPriceCurrent - sqrtPriceLower);
}
return {
amount0: BigInt(Math.floor(amount0)),
amount1: BigInt(Math.floor(amount1)),
};
}
```
Then reuse it here:
```ts
import { getAmountsForPositionApprox } from './liquidityMath';
// ...
const { amount0, amount1 } = getAmountsForPositionApprox(
liquidity,
tickLower,
tickUpper,
currentTick,
);
```
And in `GooddollarLiquidityWidget`:
```ts
import { getAmountsForPositionApprox } from '../pool/liquidityMath';
// replace local range/price math with the shared helper
const { amount0, amount1 } = getAmountsForPositionApprox(
liquidity,
tickLower,
tickUpper,
currentTick,
);
```
This keeps all the tricky branching and tick/price math in one well-named function, reduces duplication across files, and makes the bigint→number approximation decision explicit and documented in a single place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
sirpy
left a comment
There was a problem hiding this comment.
- why is number used and not bigint? i think bigint can be used everywhere where required and not scaled down.
- separate into an sdk package and a widget package (see savings-wdiget and savings-sdk
- can you add a react example / docs please
sirpy
left a comment
There was a problem hiding this comment.
Please also add a demo react app in the apps folder
| if (val === 0n) return '0'; | ||
| const [whole, frac = ''] = formatEther(val).split('.'); | ||
| if (whole === '0' && !/[1-9]/.test(frac.slice(0, 4))) return '<0.0001'; | ||
| const grouped = whole.replace(/\B(?=(\d{3})+(?!\d))/g, ','); | ||
| const trimmedFrac = frac.slice(0, 4).replace(/0+$/, ''); | ||
| return trimmedFrac ? `${grouped}.${trimmedFrac}` : grouped; |
There was a problem hiding this comment.
dont understand what this is
| const scaledSqrt = (sqrtPriceX96 * pow10(scale)) / Q96; | ||
| return toFiniteNumber(formatFixed(scaledSqrt, scale)); | ||
| const scaled = (sqrtPriceX96 * SCALE) / Q96; | ||
| return Number(scaled) / 1e18; |
There was a problem hiding this comment.
use Number(formatEther(scaled)) to convert from bigint to number/1e18
| if (sqrtPriceX96 === 0n) return 0; | ||
| const scale = 18; | ||
| const scaledPrice = (sqrtPriceX96 * sqrtPriceX96 * pow10(scale)) / Q192; | ||
| const rawPrice = toFiniteNumber(formatFixed(scaledPrice, scale)); | ||
| const scaledPrice = (sqrtPriceX96 * sqrtPriceX96 * SCALE) / Q192; | ||
| const rawPrice = Number(scaledPrice) / 1e18; |
There was a problem hiding this comment.
this code is same as computeSqrtPriceFloat function, you can replace
|
|
||
| ```ts | ||
| // src/types/liquidity-widget.d.ts | ||
| import type { GooddollarLiquidityWidget } from "@goodsdks/liquidity-widget/src/GooddollarLiquidityWidget"; |
|
🤖 Automated comment from PullFlow setup. |
|
@kadiray34 I will review now but next time please make sure to include comments and feedback back towards the comments raised to make sure things are accounted for and we are aligned |
L03TJ3
left a comment
There was a problem hiding this comment.
Approved, just please include the iife for the webcomponent.
@kadiray34
|
|
||
| This component can be used on 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. |
There was a problem hiding this comment.
iife should be included so that we can link it through releases
Description
This widget is for adding G$-USDGLO liquidity on Ubeswap. Users can add liquidity and manage through this widget.
Summary by Sourcery
Add a reusable web component for providing G$/USDGLO liquidity on Ubeswap, including wallet integration, range selection, transaction handling, and position display.
New Features:
Enhancements:
Build:
Documentation: