Savings Widget improvements#49
Open
kadiray34 wants to merge 8 commits into
Open
Conversation
Agent-Logs-Url: https://github.com/GoodDollar/GoodSDKs/sessions/3aa3109f-691f-4527-ba8f-f9a5ae0c76b8 Co-authored-by: L03TJ3 <6606028+L03TJ3@users.noreply.github.com>
Agent-Logs-Url: https://github.com/GoodDollar/GoodSDKs/sessions/3aa3109f-691f-4527-ba8f-f9a5ae0c76b8 Co-authored-by: L03TJ3 <6606028+L03TJ3@users.noreply.github.com>
Agent-Logs-Url: https://github.com/GoodDollar/GoodSDKs/sessions/3aa3109f-691f-4527-ba8f-f9a5ae0c76b8 Co-authored-by: L03TJ3 <6606028+L03TJ3@users.noreply.github.com>
![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 4 issues, and left some high level feedback:
- The
supported-chainsprop is documented as accepting a JSON array string, but Lit’s defaulttype: Arrayconverter won’t JSON-parse the attribute; consider adding a custom converter or enforcing thatsupportedChainsis only set via JS to avoid unexpected string parsing at runtime. - There is duplicated logic for resolving the active chain ID between
resolveActiveChainId()andrefreshData()(walletChain vs default/supported chains); centralizing this into a single helper that accepts an optionalwalletChainIdwould make the network selection logic easier to reason about and less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `supported-chains` prop is documented as accepting a JSON array string, but Lit’s default `type: Array` converter won’t JSON-parse the attribute; consider adding a custom converter or enforcing that `supportedChains` is only set via JS to avoid unexpected string parsing at runtime.
- There is duplicated logic for resolving the active chain ID between `resolveActiveChainId()` and `refreshData()` (walletChain vs default/supported chains); centralizing this into a single helper that accepts an optional `walletChainId` would make the network selection logic easier to reason about and less error-prone.
## Individual Comments
### Comment 1
<location path="apps/demo-savings-widget/src/index.js" line_range="18-26" />
<code_context>
+ return
+ }
+
+ const accounts = await provider.request({ method: "eth_accounts" })
+ if (accounts?.length) {
+ const [account] = accounts
+ walletStatus.textContent = `Connected: ${account.slice(0, 6)}...${account.slice(-4)}`
</code_context>
<issue_to_address>
**suggestion:** `eth_accounts` call in the demo is unguarded and can throw, which would leave the UI in an inconsistent state.
In `updateWalletStatus`, if `provider.request({ method: "eth_accounts" })` throws (e.g. provider error), the exception will bubble up and prevent the status text from updating. Since this is example code for integrators, consider wrapping the call in try/catch and setting a clear error message so the demo stays in a consistent state even when the injected wallet misbehaves.
```suggestion
try {
const accounts = await provider.request({ method: "eth_accounts" })
if (accounts?.length) {
const [account] = accounts
walletStatus.textContent = `Connected: ${account.slice(0, 6)}...${account.slice(-4)}`
savingsWidget.web3Provider = provider
} else {
walletStatus.textContent = "Wallet not connected"
savingsWidget.web3Provider = null
}
} catch (error) {
console.error("Failed to read accounts from injected wallet", error)
walletStatus.textContent = "Error reading accounts from wallet. Please check your browser wallet and try again."
savingsWidget.web3Provider = null
}
```
</issue_to_address>
### Comment 2
<location path="packages/savings-widget/README.md" line_range="65" />
<code_context>
Defines the function 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 handeled outside of this component.
\ No newline at end of file
+ The web3Provider object when the wallet is connected. Wallet connection logic should be handeled outside of this component.
</code_context>
<issue_to_address>
**issue (typo):** Typo in 'handeled' – should be 'handled'.
In that sentence, update the word to "handled".
```suggestion
The web3Provider object when the wallet is connected. Wallet connection logic should be handled outside of this component.
```
</issue_to_address>
### Comment 3
<location path="packages/savings-widget/src/GooddollarSavingsWidget.ts" line_range="593" />
<code_context>
`;
}
+ private resolveActiveChainId(): number {
+ const supported = this.getSupportedChainsSafe();
+ if (
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring active-chain resolution, supported-chain normalization, provider listener wiring, and network checks to reuse shared helpers and cached state instead of duplicating logic in multiple places.
You can trim a fair bit of complexity without losing any functionality by:
### 1. Centralizing active-chain resolution
`refreshData()` re‑implements the same priority logic already encoded in `resolveActiveChainId()`. You can make `resolveActiveChainId` accept an explicit `walletChainId` and reuse it from `refreshData` instead of duplicating the branching:
```ts
private resolveActiveChainId(walletChainId: number | null = this.walletChainId): number {
const supported = this.getSupportedChainsSafe();
if (walletChainId !== null && supported.includes(walletChainId)) {
return walletChainId;
}
if (supported.includes(this.defaultChainId)) {
return this.defaultChainId;
}
return supported[0] ?? DEFAULT_CHAIN_ID;
}
private async refreshData() {
const supported = this.getSupportedChainsSafe();
let walletChainId: number | null = null;
if (this.web3Provider?.request) {
try {
const chainIdHex = await this.web3Provider.request({ method: 'eth_chainId' });
walletChainId = parseInt(chainIdHex, 16);
} catch (error) {
console.error('Failed to read wallet chain id:', error);
walletChainId = null;
}
}
this.walletChainId = walletChainId;
const previousActive = this.activeChainId;
const nextActive = this.resolveActiveChainId(walletChainId);
if (nextActive !== previousActive) {
this.activeChainId = nextActive;
this.sdk = null;
this.resetUserStats();
}
const activeConfig = getSavingsChainConfig(this.activeChainId);
// ... rest of refreshData unchanged
}
```
This keeps all behavior while ensuring there’s exactly one place that defines “how activeChainId is chosen”.
### 2. Simplifying supported-chains normalization
`getSupportedChainsSafe` currently does `Number(id)` conversion twice. Given the property type is already `number[]`, you can drop the redundant conversions:
```ts
private getSupportedChainsSafe(): number[] {
const list = (this.supportedChains ?? []).filter((id) =>
isSupportedChainId(id),
);
return list.length > 0 ? list : DEFAULT_SUPPORTED_CHAIN_IDS;
}
```
If you want to be extra defensive against non‑numeric attribute values, normalize once:
```ts
private getSupportedChainsSafe(): number[] {
const raw = this.supportedChains ?? [];
const list = raw
.map((id) => Number(id))
.filter((id) => Number.isInteger(id) && isSupportedChainId(id));
return list.length > 0 ? list : DEFAULT_SUPPORTED_CHAIN_IDS;
}
```
Either way, you avoid the filter+map double conversion.
### 3. Reducing provider listener state
You don’t really need `providerListenersAttachedTo`; you can attach/detach directly on the current `web3Provider` and use the presence of the handlers as the “attached” flag. This removes one mutable field and simplifies the lifecycle:
```ts
private attachProviderListeners() {
if (!this.web3Provider?.on || this.chainChangedHandler) return;
this.chainChangedHandler = (chainIdHex: string) => {
try {
this.walletChainId = parseInt(chainIdHex, 16);
} catch {
this.walletChainId = null;
}
this.refreshData().catch(console.error);
};
this.accountsChangedHandler = (accounts: string[]) => {
this.userAddress = accounts?.[0] ?? null;
this.refreshData().catch(console.error);
};
this.web3Provider.on('chainChanged', this.chainChangedHandler);
this.web3Provider.on('accountsChanged', this.accountsChangedHandler);
}
private detachProviderListeners() {
if (!this.web3Provider?.removeListener) {
this.chainChangedHandler = null;
this.accountsChangedHandler = null;
return;
}
if (this.chainChangedHandler) {
this.web3Provider.removeListener('chainChanged', this.chainChangedHandler);
}
if (this.accountsChangedHandler) {
this.web3Provider.removeListener('accountsChanged', this.accountsChangedHandler);
}
this.chainChangedHandler = null;
this.accountsChangedHandler = null;
}
```
`updated` can stay the same:
```ts
if (changedProperties.has('web3Provider')) {
this.detachProviderListeners();
this.attachProviderListeners();
this.refreshData();
}
```
Behavior is preserved, but the listener state is much easier to reason about.
### 4. Avoiding duplicate network checks
`refreshData` already fetches and stores `walletChainId`, while `ensureActiveNetwork` does an extra `eth_chainId` call. You can centralize this with a small helper and reuse the cached value when present:
```ts
private async getWalletChainId(): Promise<number | null> {
if (!this.web3Provider?.request) return null;
try {
const hex = await this.web3Provider.request({ method: 'eth_chainId' });
return parseInt(hex, 16);
} catch (e) {
console.error('Failed to read wallet chain id:', e);
return null;
}
}
private async refreshData() {
const supported = this.getSupportedChainsSafe();
const walletChainId = await this.getWalletChainId();
this.walletChainId = walletChainId;
const nextActive = this.resolveActiveChainId(walletChainId);
// ... rest as above
}
private async ensureActiveNetwork(): Promise<boolean> {
if (!this.web3Provider?.request) return true;
const currentChainId =
this.walletChainId !== null ? this.walletChainId : await this.getWalletChainId();
if (currentChainId === this.activeChainId) return true;
const targetHex = `0x${this.activeChainId.toString(16)}`;
try {
await this.web3Provider.request({
method: 'wallet_switchEthereumChain',
params: [{ chainId: targetHex }],
});
this.transactionError = '';
this.walletChainId = this.activeChainId;
return true;
} catch (error: any) {
this.transactionError = this.toUserErrorMessage(
error,
`Please switch your wallet to ${this.getChainName(this.activeChainId)}.`,
);
return false;
}
}
```
This removes the second `eth_chainId` round‑trip while making “what network are we on vs what network should we be on” follow a single path.
</issue_to_address>
### Comment 4
<location path="packages/savings-sdk/src/viem-sdk.ts" line_range="373" />
<code_context>
- async unstake(amount: bigint, onHash?: (hash: `0x${string}`) => void) {
- if (amount <= BigInt(0)) throw new Error("Amount must be greater than zero")
+ private async stakeStreaming(
+ amount: bigint,
+ onHash?: (hash: `0x${string}`) => void,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the Superfluid batch operation construction and precomputing contract ABI/addresses to keep `stakeStreaming` and contract calls higher-level and easier to read.
You can reduce some of the new complexity without changing behavior by:
---
### 1. Encapsulate Superfluid batch operations
`stakeStreaming` interleaves business logic with low‑level batch op details. You can keep the flow readable by extracting the operation construction into small helpers:
```ts
private buildApproveOp(): {
operationType: number
target: `0x${string}`
data: `0x${string}`
} {
return {
operationType: OP_TYPE_ERC20_APPROVE,
target: this.contracts.gdollar,
data: encodeAbiParameters(
[{ type: "address" }, { type: "uint256" }],
[this.contracts.staking, MAX_UINT256],
),
}
}
private buildConnectPoolOp(
pool: `0x${string}`,
gdaForwarder: `0x${string}`,
): {
operationType: number
target: `0x${string}`
data: `0x${string}`
} {
const connectPoolCall = encodeFunctionData({
abi: GDA_FORWARDER_ABI,
functionName: "connectPool",
args: [pool, "0x"],
})
return {
operationType: OP_TYPE_SUPERFLUID_CALL_AGREEMENT,
target: gdaForwarder,
data: encodeAbiParameters(
[{ type: "bytes" }, { type: "bytes" }],
[connectPoolCall, "0x"],
),
}
}
private buildStakeOp(
amount: bigint,
): {
operationType: number
target: `0x${string}`
data: `0x${string}`
} {
return {
operationType: OP_TYPE_ERC2771_FORWARD_CALL,
target: this.contracts.staking,
data: encodeFunctionData({
abi: STREAMING_STAKING_ABI,
functionName: "stake",
args: [amount],
}),
}
}
```
Then `stakeStreaming` becomes mostly “high‑level”:
```ts
private async stakeStreaming(
amount: bigint,
onHash?: (hash: `0x${string}`) => void,
) {
const account = await this.getAccount()
const host = this.contracts.superfluidHost!
const gdaForwarder = this.contracts.gdaForwarder!
const pool = await this.getPoolAddress()
const [allowance, isConnected] = await Promise.all([
this.publicClient.readContract({
...this.gdollarContract(),
functionName: "allowance",
args: [account, this.contracts.staking],
}),
this.publicClient.readContract({
address: gdaForwarder,
abi: GDA_FORWARDER_ABI,
functionName: "isMemberConnected",
args: [pool, account],
}),
])
const ops = []
if (allowance < amount) ops.push(this.buildApproveOp())
if (!isConnected) ops.push(this.buildConnectPoolOp(pool, gdaForwarder))
ops.push(this.buildStakeOp(amount))
return this.submitAndWait(
{
address: host,
abi: SUPERFLUID_HOST_ABI,
functionName: "batchCall",
args: [ops],
},
onHash,
)
}
```
This keeps all semantics but makes the intent of `stakeStreaming` (“ensure allowance; ensure pool connection; stake”) much clearer.
---
### 2. Simplify contract accessors by precomputing ABI/address
`stakingContract()` and `gdollarContract()` are thin wrappers that are called frequently and add an extra layer of indirection. You already have `this.contracts`; you can precompute the staking ABI once in the constructor and use simple fields instead of factory methods.
In the class:
```ts
private readonly stakingAbi: typeof CLASSIC_STAKING_ABI | typeof STREAMING_STAKING_ABI
private readonly stakingAddress: `0x${string}`
private readonly gdollarAddress: `0x${string}`
// in constructor, after this.chainConfig is set:
this.stakingAbi = this.chainConfig.isStreaming
? STREAMING_STAKING_ABI
: CLASSIC_STAKING_ABI
this.stakingAddress = this.contracts.staking
this.gdollarAddress = this.contracts.gdollar
```
Then replace the helpers:
```ts
// remove these:
// private stakingContract() { ... }
// private gdollarContract() { ... }
// inline usage instead:
const stakingContract = {
address: this.stakingAddress,
abi: this.stakingAbi,
} as const
const gdollarContract = {
address: this.gdollarAddress,
abi: G$__ABI,
} as const
```
Example usage change:
```ts
// before
this.publicClient.readContract({
...this.stakingContract(),
functionName: "totalSupply",
})
// after
this.publicClient.readContract({
address: this.stakingAddress,
abi: this.stakingAbi,
functionName: "totalSupply",
})
```
This keeps multi‑mode behavior but removes repeated `stakingContract()` / `gdollarContract()` calls and makes call sites easier to parse.
</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
+18
to
+26
| const accounts = await provider.request({ method: "eth_accounts" }) | ||
| if (accounts?.length) { | ||
| const [account] = accounts | ||
| walletStatus.textContent = `Connected: ${account.slice(0, 6)}...${account.slice(-4)}` | ||
| savingsWidget.web3Provider = provider | ||
| } else { | ||
| walletStatus.textContent = "Wallet not connected" | ||
| savingsWidget.web3Provider = null | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion: eth_accounts call in the demo is unguarded and can throw, which would leave the UI in an inconsistent state.
In updateWalletStatus, if provider.request({ method: "eth_accounts" }) throws (e.g. provider error), the exception will bubble up and prevent the status text from updating. Since this is example code for integrators, consider wrapping the call in try/catch and setting a clear error message so the demo stays in a consistent state even when the injected wallet misbehaves.
Suggested change
| const accounts = await provider.request({ method: "eth_accounts" }) | |
| if (accounts?.length) { | |
| const [account] = accounts | |
| walletStatus.textContent = `Connected: ${account.slice(0, 6)}...${account.slice(-4)}` | |
| savingsWidget.web3Provider = provider | |
| } else { | |
| walletStatus.textContent = "Wallet not connected" | |
| savingsWidget.web3Provider = null | |
| } | |
| try { | |
| const accounts = await provider.request({ method: "eth_accounts" }) | |
| if (accounts?.length) { | |
| const [account] = accounts | |
| walletStatus.textContent = `Connected: ${account.slice(0, 6)}...${account.slice(-4)}` | |
| savingsWidget.web3Provider = provider | |
| } else { | |
| walletStatus.textContent = "Wallet not connected" | |
| savingsWidget.web3Provider = null | |
| } | |
| } catch (error) { | |
| console.error("Failed to read accounts from injected wallet", error) | |
| walletStatus.textContent = "Error reading accounts from wallet. Please check your browser wallet and try again." | |
| savingsWidget.web3Provider = null | |
| } |
|
|
||
| - **`web3Provider`**: _(Set via JavaScript property)_ | ||
| The web3Provider object when the wallet is connected. Wallet connection logic should be handeled outside of this component. No newline at end of file | ||
| The web3Provider object when the wallet is connected. Wallet connection logic should be handeled outside of this component. |
Contributor
There was a problem hiding this comment.
issue (typo): Typo in 'handeled' – should be 'handled'.
In that sentence, update the word to "handled".
Suggested change
| The web3Provider object when the wallet is connected. Wallet connection logic should be handeled outside of this component. | |
| The web3Provider object when the wallet is connected. Wallet connection logic should be handled outside of this component. |
| `; | ||
| } | ||
|
|
||
| private resolveActiveChainId(): number { |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider refactoring active-chain resolution, supported-chain normalization, provider listener wiring, and network checks to reuse shared helpers and cached state instead of duplicating logic in multiple places.
You can trim a fair bit of complexity without losing any functionality by:
1. Centralizing active-chain resolution
refreshData() re‑implements the same priority logic already encoded in resolveActiveChainId(). You can make resolveActiveChainId accept an explicit walletChainId and reuse it from refreshData instead of duplicating the branching:
private resolveActiveChainId(walletChainId: number | null = this.walletChainId): number {
const supported = this.getSupportedChainsSafe();
if (walletChainId !== null && supported.includes(walletChainId)) {
return walletChainId;
}
if (supported.includes(this.defaultChainId)) {
return this.defaultChainId;
}
return supported[0] ?? DEFAULT_CHAIN_ID;
}
private async refreshData() {
const supported = this.getSupportedChainsSafe();
let walletChainId: number | null = null;
if (this.web3Provider?.request) {
try {
const chainIdHex = await this.web3Provider.request({ method: 'eth_chainId' });
walletChainId = parseInt(chainIdHex, 16);
} catch (error) {
console.error('Failed to read wallet chain id:', error);
walletChainId = null;
}
}
this.walletChainId = walletChainId;
const previousActive = this.activeChainId;
const nextActive = this.resolveActiveChainId(walletChainId);
if (nextActive !== previousActive) {
this.activeChainId = nextActive;
this.sdk = null;
this.resetUserStats();
}
const activeConfig = getSavingsChainConfig(this.activeChainId);
// ... rest of refreshData unchanged
}This keeps all behavior while ensuring there’s exactly one place that defines “how activeChainId is chosen”.
2. Simplifying supported-chains normalization
getSupportedChainsSafe currently does Number(id) conversion twice. Given the property type is already number[], you can drop the redundant conversions:
private getSupportedChainsSafe(): number[] {
const list = (this.supportedChains ?? []).filter((id) =>
isSupportedChainId(id),
);
return list.length > 0 ? list : DEFAULT_SUPPORTED_CHAIN_IDS;
}If you want to be extra defensive against non‑numeric attribute values, normalize once:
private getSupportedChainsSafe(): number[] {
const raw = this.supportedChains ?? [];
const list = raw
.map((id) => Number(id))
.filter((id) => Number.isInteger(id) && isSupportedChainId(id));
return list.length > 0 ? list : DEFAULT_SUPPORTED_CHAIN_IDS;
}Either way, you avoid the filter+map double conversion.
3. Reducing provider listener state
You don’t really need providerListenersAttachedTo; you can attach/detach directly on the current web3Provider and use the presence of the handlers as the “attached” flag. This removes one mutable field and simplifies the lifecycle:
private attachProviderListeners() {
if (!this.web3Provider?.on || this.chainChangedHandler) return;
this.chainChangedHandler = (chainIdHex: string) => {
try {
this.walletChainId = parseInt(chainIdHex, 16);
} catch {
this.walletChainId = null;
}
this.refreshData().catch(console.error);
};
this.accountsChangedHandler = (accounts: string[]) => {
this.userAddress = accounts?.[0] ?? null;
this.refreshData().catch(console.error);
};
this.web3Provider.on('chainChanged', this.chainChangedHandler);
this.web3Provider.on('accountsChanged', this.accountsChangedHandler);
}
private detachProviderListeners() {
if (!this.web3Provider?.removeListener) {
this.chainChangedHandler = null;
this.accountsChangedHandler = null;
return;
}
if (this.chainChangedHandler) {
this.web3Provider.removeListener('chainChanged', this.chainChangedHandler);
}
if (this.accountsChangedHandler) {
this.web3Provider.removeListener('accountsChanged', this.accountsChangedHandler);
}
this.chainChangedHandler = null;
this.accountsChangedHandler = null;
}updated can stay the same:
if (changedProperties.has('web3Provider')) {
this.detachProviderListeners();
this.attachProviderListeners();
this.refreshData();
}Behavior is preserved, but the listener state is much easier to reason about.
4. Avoiding duplicate network checks
refreshData already fetches and stores walletChainId, while ensureActiveNetwork does an extra eth_chainId call. You can centralize this with a small helper and reuse the cached value when present:
private async getWalletChainId(): Promise<number | null> {
if (!this.web3Provider?.request) return null;
try {
const hex = await this.web3Provider.request({ method: 'eth_chainId' });
return parseInt(hex, 16);
} catch (e) {
console.error('Failed to read wallet chain id:', e);
return null;
}
}
private async refreshData() {
const supported = this.getSupportedChainsSafe();
const walletChainId = await this.getWalletChainId();
this.walletChainId = walletChainId;
const nextActive = this.resolveActiveChainId(walletChainId);
// ... rest as above
}
private async ensureActiveNetwork(): Promise<boolean> {
if (!this.web3Provider?.request) return true;
const currentChainId =
this.walletChainId !== null ? this.walletChainId : await this.getWalletChainId();
if (currentChainId === this.activeChainId) return true;
const targetHex = `0x${this.activeChainId.toString(16)}`;
try {
await this.web3Provider.request({
method: 'wallet_switchEthereumChain',
params: [{ chainId: targetHex }],
});
this.transactionError = '';
this.walletChainId = this.activeChainId;
return true;
} catch (error: any) {
this.transactionError = this.toUserErrorMessage(
error,
`Please switch your wallet to ${this.getChainName(this.activeChainId)}.`,
);
return false;
}
}This removes the second eth_chainId round‑trip while making “what network are we on vs what network should we be on” follow a single path.
|
|
||
| async unstake(amount: bigint, onHash?: (hash: `0x${string}`) => void) { | ||
| if (amount <= BigInt(0)) throw new Error("Amount must be greater than zero") | ||
| private async stakeStreaming( |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider extracting the Superfluid batch operation construction and precomputing contract ABI/addresses to keep stakeStreaming and contract calls higher-level and easier to read.
You can reduce some of the new complexity without changing behavior by:
1. Encapsulate Superfluid batch operations
stakeStreaming interleaves business logic with low‑level batch op details. You can keep the flow readable by extracting the operation construction into small helpers:
private buildApproveOp(): {
operationType: number
target: `0x${string}`
data: `0x${string}`
} {
return {
operationType: OP_TYPE_ERC20_APPROVE,
target: this.contracts.gdollar,
data: encodeAbiParameters(
[{ type: "address" }, { type: "uint256" }],
[this.contracts.staking, MAX_UINT256],
),
}
}
private buildConnectPoolOp(
pool: `0x${string}`,
gdaForwarder: `0x${string}`,
): {
operationType: number
target: `0x${string}`
data: `0x${string}`
} {
const connectPoolCall = encodeFunctionData({
abi: GDA_FORWARDER_ABI,
functionName: "connectPool",
args: [pool, "0x"],
})
return {
operationType: OP_TYPE_SUPERFLUID_CALL_AGREEMENT,
target: gdaForwarder,
data: encodeAbiParameters(
[{ type: "bytes" }, { type: "bytes" }],
[connectPoolCall, "0x"],
),
}
}
private buildStakeOp(
amount: bigint,
): {
operationType: number
target: `0x${string}`
data: `0x${string}`
} {
return {
operationType: OP_TYPE_ERC2771_FORWARD_CALL,
target: this.contracts.staking,
data: encodeFunctionData({
abi: STREAMING_STAKING_ABI,
functionName: "stake",
args: [amount],
}),
}
}Then stakeStreaming becomes mostly “high‑level”:
private async stakeStreaming(
amount: bigint,
onHash?: (hash: `0x${string}`) => void,
) {
const account = await this.getAccount()
const host = this.contracts.superfluidHost!
const gdaForwarder = this.contracts.gdaForwarder!
const pool = await this.getPoolAddress()
const [allowance, isConnected] = await Promise.all([
this.publicClient.readContract({
...this.gdollarContract(),
functionName: "allowance",
args: [account, this.contracts.staking],
}),
this.publicClient.readContract({
address: gdaForwarder,
abi: GDA_FORWARDER_ABI,
functionName: "isMemberConnected",
args: [pool, account],
}),
])
const ops = []
if (allowance < amount) ops.push(this.buildApproveOp())
if (!isConnected) ops.push(this.buildConnectPoolOp(pool, gdaForwarder))
ops.push(this.buildStakeOp(amount))
return this.submitAndWait(
{
address: host,
abi: SUPERFLUID_HOST_ABI,
functionName: "batchCall",
args: [ops],
},
onHash,
)
}This keeps all semantics but makes the intent of stakeStreaming (“ensure allowance; ensure pool connection; stake”) much clearer.
2. Simplify contract accessors by precomputing ABI/address
stakingContract() and gdollarContract() are thin wrappers that are called frequently and add an extra layer of indirection. You already have this.contracts; you can precompute the staking ABI once in the constructor and use simple fields instead of factory methods.
In the class:
private readonly stakingAbi: typeof CLASSIC_STAKING_ABI | typeof STREAMING_STAKING_ABI
private readonly stakingAddress: `0x${string}`
private readonly gdollarAddress: `0x${string}`
// in constructor, after this.chainConfig is set:
this.stakingAbi = this.chainConfig.isStreaming
? STREAMING_STAKING_ABI
: CLASSIC_STAKING_ABI
this.stakingAddress = this.contracts.staking
this.gdollarAddress = this.contracts.gdollarThen replace the helpers:
// remove these:
// private stakingContract() { ... }
// private gdollarContract() { ... }
// inline usage instead:
const stakingContract = {
address: this.stakingAddress,
abi: this.stakingAbi,
} as const
const gdollarContract = {
address: this.gdollarAddress,
abi: G$__ABI,
} as constExample usage change:
// before
this.publicClient.readContract({
...this.stakingContract(),
functionName: "totalSupply",
})
// after
this.publicClient.readContract({
address: this.stakingAddress,
abi: this.stakingAbi,
functionName: "totalSupply",
})This keeps multi‑mode behavior but removes repeated stakingContract() / gdollarContract() calls and makes call sites easier to parse.
Contributor
|
GM @L03TJ3, @kadiray34. Can I kindly be assigned to this. I'd follow the stated guidelines in fixing it. I'm also an active contributor in the ecosystem. |
6 tasks
L03TJ3
requested changes
May 26, 2026
L03TJ3
left a comment
L03TJ3
left a comment
Collaborator
There was a problem hiding this comment.
@kadiray34 can you first follow up on the AI reviews which are relevant to fix or you can comment back on them that the reviews are wrong/irrelevant.
Also please follow up on telegram message around apparant mismatch in intent around the smart-contract.
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
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.
Savings SDK and Widget updated with these changes:
Summary by Sourcery
Add multi-chain support and streaming rewards to the GoodDollar savings SDK and widget, and provide a demo app for testing the widget with an injected wallet.
New Features:
Bug Fixes:
Enhancements:
Documentation: