Conversation
* Create fortify.yml * Update issue templates * Update CNAME * fix: upgrade @tanstack/react-query from 5.45.1 to 5.64.2 Snyk has created this PR to upgrade @tanstack/react-query from 5.45.1 to 5.64.2. See this package in npm: @tanstack/react-query See this project in Snyk: https://app.snyk.io/org/dargon789/project/bb845543-cbee-4e11-8cf9-8bfdf9205bf1?utm_source=github&utm_medium=referral&page=upgrade-pr * Create config.yml (#46) Signed-off-by: AU_gdev_19 <64915515+Dargon789@users.noreply.github.com> * Update fortify.yml Signed-off-by: AU_gdev_19 <64915515+Dargon789@users.noreply.github.com> * Update .github/ISSUE_TEMPLATE/bug_report.md Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> * Update .github/ISSUE_TEMPLATE/feature_request.md Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> --------- Signed-off-by: AU_gdev_19 <64915515+Dargon789@users.noreply.github.com> Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com> Co-authored-by: snyk-bot <snyk-bot@snyk.io> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
Summary by Sourcery Overhaul App.tsx to build a comprehensive Sequence Wallet demo application, replacing wagmi with Sequence SDK integration, adding environment/configuration management, rich wallet operations, and a structured UI with console output for interactive testing New Features: Replace wagmi-based hooks with @0xsequence wallet initialization and integration Add environment selection and dynamic wallet URLs via query parameters Implement connect, disconnect, open/close wallet and customizable connection settings Provide extensive demo actions including chain/network switching, account/balance queries, message signing, typed data signing, and transaction sending Introduce a console component and logging for viewing function outputs Add email-based auto-login via modal with validation Enhancements: Refactor UI to use design-system components and group actions thematically Initialize logger and configure default chain/network Memoize and listen to wallet events such as chain changes Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
|
|
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideReplaces the simple wagmi demo App with a full Sequence demo dapp UI wired to the Sequence wallet SDK, bumps all Sequence packages from 1.10.14 to 1.10.15, and adds CI/security workflows and GitHub issue templates. Sequence diagram for wallet connect and auth flow in new App componentsequenceDiagram
actor User
participant App
participant SequenceWallet
participant SequenceAPI
participant ETHAuthLib
User->>App: click ConnectAndAuthButton
App->>App: build connectOptions (defaultConnectOptions + authorize)
App->>SequenceWallet: connect(connectOptions)
SequenceWallet-->>App: connectDetails { connected, chainId, session, proof, email, error }
alt already connected
App->>App: detect isWalletConnected
App-->>User: console "Wallet already connected!"
else first connection
App->>App: resetConsole / appendConsoleLine "Connecting"
alt connectOptions.authorize && connectDetails.connected
App->>App: resolve apiUrl or DEFAULT_API_URL
App->>SequenceAPI: isValidETHAuthProof({ chainId, walletAddress, ethAuthProofString })
SequenceAPI-->>App: { isValid }
App->>App: appendConsoleLine "isValid (API)?: {isValid}"
App->>ETHAuthLib: decodeProof(connectDetails.proof.proofString, true)
ETHAuthLib-->>App: decodedProof
App->>SequenceWallet: utils.isValidTypedDataSignature(address, typedData, decodedProof.signature, chainId)
SequenceWallet-->>App: isValidClient
App->>App: appendConsoleLine "connected using chainId ..."
App->>App: appendConsoleLine "isValid (client)?: {isValidClient}"
end
alt connectDetails.connected
App->>App: setIsWalletConnected(true)
App->>App: appendConsoleLine "Wallet connected!"
App->>App: appendConsoleLine "shared email: {connectDetails.email}"
else connection failed
App->>App: appendConsoleLine "Failed to connect wallet - {error}"
end
end
App->>App: setConsoleLoading(false)
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
⛔ Snyk checks have failed. 22 issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello @Dargon789, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on a significant migration of a demo application from using the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 7 issues, and left some high level feedback:
- App.tsx has grown into a very large, monolithic component with a lot of side‑effectful logic and state; consider extracting reusable hooks (e.g. wallet connection / signing / transactions) and subcomponents (e.g. environment selector, connection controls, modal) to improve readability and maintainability.
- The wallet event subscription for
chainChangedis created inside auseMemowith an empty dependency array and no cleanup; this should be auseEffectthat removes the listener on unmount and/or when the wallet instance changes to avoid potential leaks or stale handlers. - State and helper typings around
email/sanitizeEmailandconsoleMsgcould be tightened:emailis typed asstring | nullbut passed to a function expectingstring, andappendConsoleLineassumesconsoleMsgis a string (concatenating with a possiblynullinitial value), so guarding or normalizing these values would prevent runtime oddities.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- App.tsx has grown into a very large, monolithic component with a lot of side‑effectful logic and state; consider extracting reusable hooks (e.g. wallet connection / signing / transactions) and subcomponents (e.g. environment selector, connection controls, modal) to improve readability and maintainability.
- The wallet event subscription for `chainChanged` is created inside a `useMemo` with an empty dependency array and no cleanup; this should be a `useEffect` that removes the listener on unmount and/or when the wallet instance changes to avoid potential leaks or stale handlers.
- State and helper typings around `email`/`sanitizeEmail` and `consoleMsg` could be tightened: `email` is typed as `string | null` but passed to a function expecting `string`, and `appendConsoleLine` assumes `consoleMsg` is a string (concatenating with a possibly `null` initial value), so guarding or normalizing these values would prevent runtime oddities.
## Individual Comments
### Comment 1
<location> `wagmi-project/src/App.tsx:112-116` </location>
<code_context>
+ const [isOpen, toggleModal] = useState(false)
+ const [warning, setWarning] = useState(false)
+
+ useMemo(() => {
+ wallet.on('chainChanged', (chainId: string) => {
+ setShowChainId(Number(BigInt(chainId)))
+ })
+ }, [])
+
+ useEffect(() => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Using useMemo for side effects and not cleaning up event listeners can cause subtle bugs and leaks.
Here, the `useMemo` callback registers the `wallet.on('chainChanged', ...)` listener. This should instead be a `useEffect` with a cleanup that removes the listener to avoid duplicated handlers across renders/hot reloads, for example:
```ts
useEffect(() => {
const handler = (chainId: string) => setShowChainId(Number(BigInt(chainId)))
wallet.on('chainChanged', handler)
return () => wallet.off?.('chainChanged', handler)
}, [wallet])
```
</issue_to_address>
### Comment 2
<location> `wagmi-project/src/App.tsx:308` </location>
<code_context>
+ appendConsoleLine(`getAddress(): ${address}`)
+
+ const provider = wallet.getProvider()
+ const accountList = provider.listAccounts()
+ appendConsoleLine(`accounts: ${JSON.stringify(accountList)}`)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Potentially missing await on provider.listAccounts() may log a Promise instead of the accounts.
If `provider.listAccounts()` is async (as with most ethers-like providers), `accountList` will be a Promise and `JSON.stringify(accountList)` won’t show the actual addresses. Use `const accountList = await provider.listAccounts()` and make this function `async` (and update its callers) so `await` is valid here.
</issue_to_address>
### Comment 3
<location> `wagmi-project/src/App.tsx:557` </location>
<code_context>
+
+ signer = signer || wallet.getSigner()
+
+ appendConsoleLine(`Transfer txn on ${signer.getChainId()} chainId`)
+
+ // NOTE: on mainnet, the balance will be of ETH value
</code_context>
<issue_to_address>
**issue (bug_risk):** Using signer.getChainId() without awaiting may interpolate a Promise into the log string.
If `signer.getChainId()` is async (as in typical ethers signers), this will log something like `Transfer txn on [object Promise] chainId`. Instead, resolve it first and log the value:
```ts
const chainId = await signer.getChainId()
appendConsoleLine(`Transfer txn on ${chainId} chainId`)
```
Make sure the containing function is `async` and callers handle the returned Promise correctly.
</issue_to_address>
### Comment 4
<location> `wagmi-project/src/App.tsx:719` </location>
<code_context>
+ // USD Coin (PoS) on Polygon
+ const address = '0x2791bca1f2de4661ed88a30c99a7a9449aa84174'
+
+ const usdc = new ethers.Contract(address, abi)
+
+ const usdSymbol = await usdc.symbol()
</code_context>
<issue_to_address>
**issue (bug_risk):** Ethers Contract is created without a runner/provider, so subsequent calls may fail at runtime.
In ethers v6, constructing a contract without a runner (`new ethers.Contract(address, abi)`) leaves it without a provider/signer, so calls like `usdc.symbol()` will throw at runtime. Since you already have a signer, pass it (or its provider) when constructing the contract, e.g.:
```ts
const usdc = new ethers.Contract(address, abi, signer)
// or, for read-only:
// const usdc = new ethers.Contract(address, abi, signer.provider)
```
</issue_to_address>
### Comment 5
<location> `wagmi-project/src/App.tsx:961` </location>
<code_context>
+ }
+ }, [email, isOpen])
+
+ const sanitizeEmail = (email: string) => {
+ // Trim unnecessary spaces
+ email = email.trim()
</code_context>
<issue_to_address>
**issue:** sanitizeEmail assumes a non-null string, but it may be called with a null value from state.
Since `email` state is `string | null`, calling `sanitizeEmail(email)` can pass `null`, and `email.trim()` will throw. Either ensure state is always a string (e.g. `useState('')`), add a null check before calling, or update `sanitizeEmail` to accept `string | null` and handle the null case safely.
</issue_to_address>
### Comment 6
<location> `wagmi-project/src/App.tsx:118-120` </location>
<code_context>
+ })
+ }, [])
+
+ useEffect(() => {
+ setIsWalletConnected(wallet.isConnected())
+ }, [wallet])
+
+ useEffect(() => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Wallet client event listeners are never removed, which may cause leaks across re-renders or hot reloads.
Within this `useEffect`, `wallet.client.onOpen` and `wallet.client.onClose` are added but never cleaned up. If the client exposes `off`/`removeListener`, store the handler references and unregister them in the effect’s cleanup function.
</issue_to_address>
### Comment 7
<location> `wagmi-project/src/App.tsx:99` </location>
<code_context>
+ sequence.initWallet(projectAccessKey, { defaultNetwork: defaultChainId, transports: { walletAppURL } })
+}
+
+// App component
+const App = () => {
+ const [consoleMsg, setConsoleMsg] = useState<null | string>(null)
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the large `App` component by extracting wallet/console logic into hooks, fixing the chainChanged subscription, and moving the email modal into its own component to keep behavior but simplify structure.
The new functionality is valuable, but the `App` component is now doing too much at once and has a lot of repetition, which will make future changes hard. You can keep all behavior and significantly reduce complexity with a couple of focused refactors:
---
### 1. Extract wallet + console logic into hooks
You repeatedly do:
- `const wallet = sequence.getWallet()`
- `const signer = wallet.getSigner()`
- `resetConsole()`, `appendConsoleLine(...)`, `setConsoleLoading(false)`
- `try { ... } catch { consoleErrorMessage() }`
This can be centralized to make each action function just express its *core* logic.
**a) `useSequenceWallet`**
Move wallet access and connection state out of `App`:
```ts
// useSequenceWallet.ts
import { useEffect, useMemo, useState } from 'react'
import { sequence } from '0xsequence'
export function useSequenceWallet() {
const [isWalletConnected, setIsWalletConnected] = useState(false)
const wallet = useMemo(() => sequence.getWallet(), [])
const provider = wallet.getProvider()
const defaultSigner = wallet.getSigner()
useEffect(() => {
setIsWalletConnected(wallet.isConnected())
}, [wallet])
return {
wallet,
provider,
defaultSigner,
isWalletConnected,
setIsWalletConnected,
}
}
```
Then in `App`:
```ts
const {
wallet,
provider,
defaultSigner,
isWalletConnected,
setIsWalletConnected,
} = useSequenceWallet()
```
Now all places where you call `sequence.getWallet()` can use `wallet` / `defaultSigner` from the hook.
---
**b) `useConsole` + `withConsole` wrapper**
Centralize console state and boilerplate:
```ts
// useConsole.ts
import { useState, useCallback } from 'react'
export function useConsole() {
const [message, setMessage] = useState<string | null>(null)
const [loading, setLoading] = useState(false)
const resetConsole = useCallback(() => setLoading(true), [])
const appendLine = useCallback((line: string, clear = false) => {
console.log(line)
setMessage(prev => (clear || !prev ? line : `${prev}\n\n${line}`))
}, [])
const setError = useCallback(() => {
setLoading(false)
setMessage('An error occurred')
}, [])
const setStatus = useCallback((connected: boolean) => {
setLoading(false)
setMessage(
connected
? 'Status: Wallet is connected :)'
: 'Status: Wallet not connected. Please connect wallet first.',
)
}, [])
const withConsole = useCallback(
async (fn: () => Promise<void>) => {
try {
resetConsole()
await fn()
setLoading(false)
} catch (e) {
console.error(e)
setError()
}
},
[resetConsole, setError],
)
return { message, loading, appendLine, setStatus, withConsole }
}
```
Then simplify actions, e.g. `getBalance`:
```ts
// in App.tsx
const { message: consoleMsg, loading: consoleLoading, appendLine, setStatus, withConsole } =
useConsole()
const getBalance = () =>
withConsole(async () => {
const account = wallet.getAddress()
const balance1 = await provider.getBalance(account)
appendLine(`balance check 1: ${balance1.toString()}`)
const balance2 = await defaultSigner.getBalance()
appendLine(`balance check 2: ${balance2.toString()}`)
})
```
All other methods (`getAccounts`, `getNetworks`, `sendETH`, `sendDAI`, etc.) can be reduced to their core logic with the same `withConsole` wrapper, removing repeated `try/catch`, `resetConsole`, and `setConsoleLoading(false)` blocks.
---
### 2. Fix non-idiomatic subscription & cleanup
You’re using `useMemo` for a side effect and not cleaning up:
```ts
useMemo(() => {
wallet.on('chainChanged', (chainId: string) => {
setShowChainId(Number(BigInt(chainId)))
})
}, [])
```
Use `useEffect` with cleanup instead:
```ts
useEffect(() => {
const handler = (chainId: string) => {
setShowChainId(Number(BigInt(chainId)))
}
wallet.on('chainChanged', handler)
return () => {
wallet.off('chainChanged', handler)
}
}, [wallet])
```
This makes the intent clearer and avoids leaking listeners if the component is remounted.
---
### 3. Extract the email modal into its own component
The email modal is self-contained UI and state (`isOpen`, `email`, `warning`, `sanitizeEmail`) that clutters `App`. You can move it out while keeping behavior unchanged:
```tsx
// EmailConnectModal.tsx
import { useState } from 'react'
import { Box, Button, Modal, Text, TextInput } from '@0xsequence/design-system'
interface Props {
open: boolean
onClose: () => void
onSubmitEmail: (email: string) => void
}
export function EmailConnectModal({ open, onClose, onSubmitEmail }: Props) {
const [email, setEmail] = useState('')
const [warning, setWarning] = useState(false)
const sanitizeEmail = (value: string) =>
/^[a-zA-Z0-9._-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,6}$/.test(value.trim())
if (!open) return null
return (
<Modal onClose={onClose} size="sm">
<Box flexDirection="column" justifyContent="space-between" height="full" padding="16">
<Box flexDirection="column">
<Box marginTop="6">
<Text marginTop="5" variant="normal" color="text80">
Auto-email login, please specify the email address
</Text>
</Box>
<Box marginTop="4">
<TextInput onChange={ev => setEmail(ev.target.value)} />
</Box>
{warning && (
<Box marginTop="6">
<Text marginTop="5" variant="normal" color="warning">
please input an email with correct format!
</Text>
</Box>
)}
<Box gap="2" marginY="4">
<Button
variant="primary"
label="Login"
onClick={() => {
if (sanitizeEmail(email)) {
setWarning(false)
onSubmitEmail(email)
onClose()
} else {
setWarning(true)
}
}}
/>
</Box>
</Box>
</Box>
</Modal>
)
}
```
Then in `App`:
```tsx
const [isEmailModalOpen, setEmailModalOpen] = useState(false)
const [email, setEmail] = useState<string | null>(null)
// ...
<Button width="full" shape="square" onClick={() => setEmailModalOpen(true)} label="Connect with Email" />
<AnimatePresence>
<EmailConnectModal
open={isEmailModalOpen}
onClose={() => setEmailModalOpen(false)}
onSubmitEmail={value => setEmail(value)}
/>
</AnimatePresence>
```
The existing `useEffect` that reacts to `email` stays the same.
---
These three changes:
- Remove repeated boilerplate from every wallet action.
- Give clear structure (`useSequenceWallet`, `useConsole`) around core business logic.
- Isolate a self-contained UI into a separate component.
They should reduce the perceived complexity of this 1300‑line file substantially without dropping any functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant update to the demo dapp, replacing the simple wagmi-based app with a full-featured Sequence wallet demo. It also includes version bumps for all packages to 1.10.15, and adds new CI configurations and issue templates. My review focuses on the new demo application and the CI configuration.
I've identified a critical security issue with hardcoded access keys in the demo app, which must be addressed. Additionally, there are several high-severity issues related to memory leaks from improper use of React hooks and a monolithic component structure that will impact maintainability. The new CircleCI configuration also appears to be a placeholder with incorrect values that needs to be fixed or removed.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
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
1 participant
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.
df86471
4dbfca2
ffd17e6
f342784
Summary by Sourcery
Introduce a full-featured Sequence wallet demo dapp UI and upgrade the monorepo to version 1.10.15 with a new utility function, along with CI and issue template additions.
New Features:
Enhancements:
CI:
Documentation: