Conversation
|
You've used up your 5 PR reviews for this month under the Korbit Starter Plan. You'll get 5 more reviews on August 19th, 2025 or you can upgrade to Pro for unlimited PR reviews and enhanced features in your Korbit Console. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `packages/citizen-sdk/src/sdks/viem-claim-sdk.ts:74` </location>
<code_context>
this.walletClient = walletClient
this.identitySDK = identitySDK
- this.account = account ?? walletClient.account.address
+ this.account = account ?? walletClient.account
+ this.address = address ?? this.account.address
this.rdu = rdu
</code_context>
<issue_to_address>
Defaulting logic for account and address may not handle all edge cases.
If walletClient.account is undefined, address may also be undefined. Please add error handling or validation to ensure both values are properly set.
</issue_to_address>
### Comment 2
<location> `packages/citizen-sdk/src/sdks/viem-claim-sdk.ts:96` </location>
<code_context>
): Promise<ClaimSDK> {
- const [account] = await props.walletClient.getAddresses()
- return new ClaimSDK({ account, ...props })
+ const [address] = await props.walletClient.getAddresses()
+ return new ClaimSDK({
+ account: props.walletClient?.account,
+ address,
</code_context>
<issue_to_address>
The init method now passes both account and address, but account may be undefined.
Validate that walletClient.account is defined before creating the SDK to prevent downstream issues.
</issue_to_address>
### Comment 3
<location> `packages/citizen-sdk/src/sdks/viem-claim-sdk.ts:159` </location>
<code_context>
+ try {
+ let hash: Hash
+
+ if ("signMessage" in this.account) {
+ const callData = encodeFunctionData({
+ abi: params.abi,
+ functionName: params.functionName,
+ args: params.args,
+ })
+
+ // Local signing path (works on RPCs that block eth_sendTransaction)
+ hash = await this.walletClient.request({
+ // Pass the LocalAccount, not just the address
+ account: account as LocalAccount,
+ to: request.address!,
+ data: callData,
</code_context>
<issue_to_address>
Type assertion to LocalAccount may be unsafe if account is not actually a LocalAccount.
Add a runtime check to ensure 'account' is a LocalAccount before casting to prevent potential errors.
</issue_to_address>
### Comment 4
<location> `packages/citizen-sdk/src/sdks/viem-identity-sdk.ts:57` </location>
<code_context>
*/
export class IdentitySDK {
- public account: Address
+ public account: Account
+ public address: Address
publicClient: PublicClient
</code_context>
<issue_to_address>
Consider refactoring to use a unified Signer abstraction for message signing and address handling.
```suggestion
// 1) Define a simple Signer interface and unify Account vs WalletClient signing:
type Signer = {
address: Address
signMessage(opts: { message: string }): Promise<`0x${string}`>
}
// 2) In your IdentitySDK, replace both `account: Account` and `address: Address` fields
// with a single private `signer: Signer`.
export class IdentitySDK {
private signer: Signer
publicClient: PublicClient
walletClient: WalletClient & WalletActions
contract: IdentityContract
env: contractEnv = "production"
constructor({
account,
publicClient,
walletClient,
env,
}: Omit<IdentitySDKOptions, "address"> & { account?: Account }) {
if (!walletClient.account) {
throw new Error("ClaimSDK: WalletClient must have an account attached.")
}
this.publicClient = publicClient
this.walletClient = walletClient
this.env = env!
// pick either a LocalAccount or wrap the WalletClient.account into Signer
const acct = account ?? walletClient.account!
if ("signMessage" in acct) {
this.signer = acct
} else {
this.signer = {
address: acct.address,
signMessage: ({ message }) =>
walletClient.signMessage({ account: acct.address, message }),
}
}
const { contractEnvAddresses } = resolveChainAndContract(walletClient, env)
this.contract = initializeIdentityContract(
this.publicClient,
contractEnvAddresses.identityContract,
)
}
// 3) Simplify generateFVLink by always using this.signer:
async generateFVLink(
popupMode = false,
callbackUrl?: string,
chainId?: number,
): Promise<string> {
try {
const address = this.signer.address
if (!address) throw new Error("No wallet address found.")
const nonce = Math.floor(Date.now() / 1000).toString()
const fvSigMessage = FV_IDENTIFIER_MSG2.replace("<account>", address)
const fvSig = await this.signer.signMessage({ message: fvSigMessage })
const { identityUrl } = Envs[this.env]!
if (!identityUrl) throw new Error("identityUrl is not defined.")
if (!popupMode && !callbackUrl)
throw new Error("Callback URL is required for redirect mode.")
const url = new URL(identityUrl)
const params: Record<string, string | number> = {
account: address,
nonce,
fvsig: fvSig,
chain: chainId ?? (await this.publicClient.getChainId()),
}
if (callbackUrl) {
params[popupMode ? "cbu" : "rdu"] = callbackUrl
}
Object.entries(params).forEach(([k, v]) =>
url.searchParams.append(k, String(v)),
)
return url.toString()
} catch (err: any) {
console.error("generateFVLink Error:", err)
throw err
}
}
// 4) Update submitAndWait to use `this.signer.address` as well:
async submitAndWait(
params: SimulateContractParameters,
onHash?: (hash: `0x${string}`) => void,
): Promise<any> {
if (!this.signer.address) {
throw new Error("No active wallet address found.")
}
const { request } = await this.publicClient.simulateContract({
account: this.signer.address,
...params,
})
const hash = await this.walletClient.writeContract(request)
onHash?.(hash)
return waitForTransactionReceipt(this.publicClient, { hash })
}
}
```
• Introduces a single `Signer` abstraction instead of keeping separate `account` and `address`.
• Moves all branching logic into the constructor.
• Simplifies `generateFVLink` and `submitAndWait` to always use `this.signer`.
• Eliminates nested `if/else` and try/catch inside message signing while preserving both LocalAccount and WalletClient behavior.
</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
+96
to
+97
| const [address] = await props.walletClient.getAddresses() | ||
| return new ClaimSDK({ |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The init method now passes both account and address, but account may be undefined.
Validate that walletClient.account is defined before creating the SDK to prevent downstream issues.
Comment on lines
+159
to
+168
| if ("signMessage" in this.account) { | ||
| const callData = encodeFunctionData({ | ||
| abi: params.abi, | ||
| functionName: params.functionName, | ||
| args: params.args, | ||
| }) | ||
|
|
||
| // Local signing path (works on RPCs that block eth_sendTransaction) | ||
| hash = await this.walletClient.request({ | ||
| // Pass the LocalAccount, not just the address |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Type assertion to LocalAccount may be unsafe if account is not actually a LocalAccount.
Add a runtime check to ensure 'account' is a LocalAccount before casting to prevent potential errors.
| public account: Account | ||
| public address: Address | ||
| publicClient: PublicClient | ||
| walletClient: WalletClient & WalletActions |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider refactoring to use a unified Signer abstraction for message signing and address handling.
Suggested change
| walletClient: WalletClient & WalletActions | |
| // 1) Define a simple Signer interface and unify Account vs WalletClient signing: | |
| type Signer = { | |
| address: Address | |
| signMessage(opts: { message: string }): Promise<`0x${string}`> | |
| } | |
| // 2) In your IdentitySDK, replace both `account: Account` and `address: Address` fields | |
| // with a single private `signer: Signer`. | |
| export class IdentitySDK { | |
| private signer: Signer | |
| publicClient: PublicClient | |
| walletClient: WalletClient & WalletActions | |
| contract: IdentityContract | |
| env: contractEnv = "production" | |
| constructor({ | |
| account, | |
| publicClient, | |
| walletClient, | |
| env, | |
| }: Omit<IdentitySDKOptions, "address"> & { account?: Account }) { | |
| if (!walletClient.account) { | |
| throw new Error("ClaimSDK: WalletClient must have an account attached.") | |
| } | |
| this.publicClient = publicClient | |
| this.walletClient = walletClient | |
| this.env = env! | |
| // pick either a LocalAccount or wrap the WalletClient.account into Signer | |
| const acct = account ?? walletClient.account! | |
| if ("signMessage" in acct) { | |
| this.signer = acct | |
| } else { | |
| this.signer = { | |
| address: acct.address, | |
| signMessage: ({ message }) => | |
| walletClient.signMessage({ account: acct.address, message }), | |
| } | |
| } | |
| const { contractEnvAddresses } = resolveChainAndContract(walletClient, env) | |
| this.contract = initializeIdentityContract( | |
| this.publicClient, | |
| contractEnvAddresses.identityContract, | |
| ) | |
| } | |
| // 3) Simplify generateFVLink by always using this.signer: | |
| async generateFVLink( | |
| popupMode = false, | |
| callbackUrl?: string, | |
| chainId?: number, | |
| ): Promise<string> { | |
| try { | |
| const address = this.signer.address | |
| if (!address) throw new Error("No wallet address found.") | |
| const nonce = Math.floor(Date.now() / 1000).toString() | |
| const fvSigMessage = FV_IDENTIFIER_MSG2.replace("<account>", address) | |
| const fvSig = await this.signer.signMessage({ message: fvSigMessage }) | |
| const { identityUrl } = Envs[this.env]! | |
| if (!identityUrl) throw new Error("identityUrl is not defined.") | |
| if (!popupMode && !callbackUrl) | |
| throw new Error("Callback URL is required for redirect mode.") | |
| const url = new URL(identityUrl) | |
| const params: Record<string, string | number> = { | |
| account: address, | |
| nonce, | |
| fvsig: fvSig, | |
| chain: chainId ?? (await this.publicClient.getChainId()), | |
| } | |
| if (callbackUrl) { | |
| params[popupMode ? "cbu" : "rdu"] = callbackUrl | |
| } | |
| Object.entries(params).forEach(([k, v]) => | |
| url.searchParams.append(k, String(v)), | |
| ) | |
| return url.toString() | |
| } catch (err: any) { | |
| console.error("generateFVLink Error:", err) | |
| throw err | |
| } | |
| } | |
| // 4) Update submitAndWait to use `this.signer.address` as well: | |
| async submitAndWait( | |
| params: SimulateContractParameters, | |
| onHash?: (hash: `0x${string}`) => void, | |
| ): Promise<any> { | |
| if (!this.signer.address) { | |
| throw new Error("No active wallet address found.") | |
| } | |
| const { request } = await this.publicClient.simulateContract({ | |
| account: this.signer.address, | |
| ...params, | |
| }) | |
| const hash = await this.walletClient.writeContract(request) | |
| onHash?.(hash) | |
| return waitForTransactionReceipt(this.publicClient, { hash }) | |
| } | |
| } |
• Introduces a single Signer abstraction instead of keeping separate account and address.
• Moves all branching logic into the constructor.
• Simplifies generateFVLink and submitAndWait to always use this.signer.
• Eliminates nested if/else and try/catch inside message signing while preserving both LocalAccount and WalletClient behavior.
Contributor
|
@L03TJ3 can we get more info about what this solves? an example of a wallet without signing? why can't a walletclient with Account be used instead? |
Collaborator
Author
|
@sirpy thats not the problem thats being solved. |
Contributor
the code wraps signmessage around walletclient or Account, i dont see any reason why it wouldnt work. |
Contributor
5 tasks
This branch was successfully deployed
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
To remove maintance of sdk methods in separated places, the handling of a potential connected custodial wallet is merged into the core sdk.
Also includes:
About # (link your issue here)
#14 (comment)
How Has This Been Tested?
Tested flow in demo.