-
Notifications
You must be signed in to change notification settings - Fork 4
feat: public transactions, token fetch bug fix #92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe pull request introduces modifications to several components and hooks in the web application, primarily focusing on simplifying the chain and account-related logic. The changes remove dependencies on the Changes
Sequence DiagramsequenceDiagram
participant User
participant App
participant Wallet
participant Chain
User->>App: Access Transactions Page
App->>Chain: Retrieve DEFAULT_CHAIN
App->>Wallet: Check Connection Status
App->>App: Render Transactions or Connect Wallet Prompt
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for kleros-escrow-v2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
web/src/hooks/useNativeTokenSymbol.ts (1)
1-1
: Use a fallback to protect against undefined chain.While
getChain(DEFAULT_CHAIN)
should nominally return a valid object, it might be safer to handle the scenario where it returnsundefined
. This prevents runtime errors ifDEFAULT_CHAIN
gets changed or removed in the future.export const useNativeTokenSymbol = () => { - return getChain(DEFAULT_CHAIN)?.nativeCurrency.symbol; + const chain = getChain(DEFAULT_CHAIN); + return chain ? chain.nativeCurrency.symbol : "UNKNOWN"; };web/src/hooks/useTokenMetadata.ts (1)
12-13
: Consider allowing the 'native' token usage scenario.While this snippet correctly returns early if
tokenAddress === "native"
, you might also want to support retrieving metadata for the native token in the future. If so, consider implementing an alternate handling path for a'native'
address.web/src/pages/MyTransactions/index.tsx (1)
36-50
: Simplify or rename file to reflect that Dashboard is always accessible.Previously, this component included connection checks. Now it always renders the routes unconditionally. Consider clarifying in its name or documentation that this is purely a container for transaction routes.
web/src/pages/MyTransactions/TransactionsFetcher.tsx (1)
39-40
: Chain ID check is correct.You verify the current chain is the default chain. This ensures the user is indeed on the supported network. For multi-chain expansion in the future, consider referencing a list of supported chains rather than just a single default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/src/hooks/useNativeTokenSymbol.ts
(1 hunks)web/src/hooks/useTokenMetadata.ts
(2 hunks)web/src/pages/MyTransactions/TransactionsFetcher.tsx
(4 hunks)web/src/pages/MyTransactions/index.tsx
(2 hunks)
🔇 Additional comments (6)
web/src/hooks/useNativeTokenSymbol.ts (1)
4-4
: Implementation looks concise.
The function correctly returns the native currency symbol using the default chain configuration. This aligns with the broader shift away from using useAccount
.
web/src/hooks/useTokenMetadata.ts (2)
3-4
: Removal of conditional usage of chain is consistent with the PR direction.
You are now referencing DEFAULT_CHAIN
directly instead of relying on user-based chain detection. This simplifies the logic, especially if multiple chains aren’t supported at this time.
24-24
: Dependency array check.
Make sure you’ve validated that the removal of chain-based dependencies won’t cause data to become stale if DEFAULT_CHAIN
changes dynamically in the future. Right now, it’s presumably a single constant, so it’s fine, but if you anticipate multiple chain usage later, reintroducing chain in the dependency array might become necessary.
web/src/pages/MyTransactions/index.tsx (1)
23-24
: Styling looks good.
Applying responsive styles here is consistent with the codebase’s approach to styling.
web/src/pages/MyTransactions/TransactionsFetcher.tsx (2)
9-9
: DEFAULT_CHAIN import is consistent with PR approach.
We see that you’ve introduced default chain logic here. It’s coherent with the other updates in this pull request.
Line range hint 81-96
: User guidance is well-handled.
The fallback UI for connecting the wallet and switching chains is appropriate. This ensures a clear user flow for unsupported chain cases.
PR-Codex overview
This PR focuses on refactoring the handling of the native token symbol and chain validation in various hooks and components, replacing direct account access with a default chain reference.
Detailed summary
useNativeTokenSymbol
, replaced account-based chain retrieval withgetChain(DEFAULT_CHAIN)
.useTokenMetadata
, removed dependency onchain
and adjusted Alchemy configuration to useDEFAULT_CHAIN
.Dashboard
component by removing account connection checks.TransactionsFetcher
, added default chain check for connected status.Summary by CodeRabbit
New Features
Refactor
Bug Fixes