-
Notifications
You must be signed in to change notification settings - Fork 9
feat: add warning regarding smart contract wallet usage #123
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
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for escrow ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a dismissible SmartContractWalletWarning React component mounted in the app header that detects smart-contract wallets via Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App/Header
participant SCW as SmartContractWalletWarning
participant Web3 as web3.eth
participant LS as localStorage
participant User
App->>SCW: Mount component in header
SCW->>LS: Read "@kleros/escrow/alert/smart-contract-wallet-warning:<account>"
alt no stored value
SCW->>SCW: showWarning = true
else stored value
SCW->>SCW: showWarning = parsed stored value (boolean)
end
Note over SCW,Web3: On account/library change or mount
SCW->>Web3: getCode(account)
Web3-->>SCW: bytecode (e.g., 0x... or 0x)
SCW->>SCW: isEip7702Eoa = bytecode.toLowerCase().startsWith("0xef0100")
alt bytecode == "0x" or isEip7702Eoa
SCW->>SCW: isSmartContractWallet = false
SCW-->>App: Render nothing
else
SCW->>SCW: isSmartContractWallet = true
SCW-->>App: Render top warning banner
end
User-->>SCW: Click dismiss (X)
SCW->>SCW: showWarning = false
SCW->>LS: Set "@kleros/escrow/alert/smart-contract-wallet-warning:<account>" = false
SCW-->>App: Hide banner
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 4
🧹 Nitpick comments (5)
src/components/sc-wallet-warning/sc-wallet-warning.css (2)
1-17
: Fixed banner may overlap header/content; reserve vertical space or move component outside header.Because the banner is
position: fixed; top: 0;
and inserted inside<header>
, it can visually overlap the header/logo or the page’s first content. Consider either:
- Rendering the banner just inside
.App
and before<header>
; or- Reserving space when visible (e.g., add top padding to
main
/body
equal to the banner height).If you keep it fixed, adding a spacer when visible avoids layout shift and overlap.
Also applies to: 19-23
24-31
: Improve close button usability and accessibility.
- Increase hit area for touch (recommended 32–44px).
- Add focus-visible styles for keyboard users.
- Ensure sufficient color contrast on default and hover states.
Example CSS tweaks:
.sc-wallet-warning button { background: transparent; border: none; cursor: pointer; - color: rgba(0, 0, 0, 0.45); - font-size: 16px; - line-height: 1; - padding: 0 4px; + color: rgba(0, 0, 0, 0.65); + font-size: 16px; + line-height: 1; + width: 32px; + height: 32px; + display: inline-flex; + align-items: center; + justify-content: center; + padding: 0; + border-radius: 4px; } .sc-wallet-warning button:hover { - color: rgba(0, 0, 0, 0.75); + color: rgba(0, 0, 0, 0.85); +} + +.sc-wallet-warning button:focus-visible { + outline: 2px solid #2f54eb; + outline-offset: 2px; }Also applies to: 33-34
src/bootstrap/app.js (1)
71-72
: Consider rendering the banner outside<header>
to avoid stacking/layout issues.Placing a fixed banner inside the header can interfere with header layout and z-index. Rendering it just inside
.App
and before<header>
is often safer:function Main({ children }) { return ( <div className="App"> - <header className="header"> - <SmartContractWalletWarning /> + <SmartContractWalletWarning /> + <header className="header">src/components/sc-wallet-warning/index.js (2)
27-30
: Persist booleans as JSON and handle storage errors.Store a string via
JSON.stringify(false)
and wrap in try/catch to avoid QUOTA/Privacy errors.const handleClose = () => { setShowWarning(false); - localStorage.setItem(storageKey, false); + try { + localStorage.setItem(storageKey, JSON.stringify(false)); + } catch {} }
33-37
: A11y: add role and labels; improve semantics of the dismiss control.Give the container
role="alert"
(oraria-live="polite"
) and the button an accessible name plus type.- return ( - <div className="sc-wallet-warning"> - <p>You are using a smart contract wallet. This is not recommended.</p> - <button onClick={handleClose}>X</button> - </div> - ); + return ( + <div className="sc-wallet-warning" role="alert" aria-live="polite"> + <p>You are using a smart contract wallet. This is not recommended.</p> + <button + type="button" + aria-label="Dismiss smart contract wallet warning" + onClick={handleClose} + > + × + </button> + </div> + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/bootstrap/app.js
(2 hunks)src/components/sc-wallet-warning/index.js
(1 hunks)src/components/sc-wallet-warning/sc-wallet-warning.css
(1 hunks)src/components/sc-wallet-warning/sc-wallet-warning.scss
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/bootstrap/app.js (1)
src/components/sc-wallet-warning/index.js (1)
SmartContractWalletWarning
(8-38)
src/components/sc-wallet-warning/index.js (1)
src/bootstrap/dapp-api.js (1)
web3
(12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - escrow
- GitHub Check: Header rules - escrow
- GitHub Check: Pages changed - escrow
🔇 Additional comments (1)
src/bootstrap/app.js (1)
17-17
: Import location is fine; ensure the component does not rely on a different Web3 instance.Given
Web3ReactProvider
supplies a Web3 instance via context, prefer using that inside the warning component instead of importing a separateweb3
fromdapp-api
to avoid provider mismatches.
746e604
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: 1
♻️ Duplicate comments (3)
src/components/sc-wallet-warning/index.js (3)
3-3
: Replace static web3 with the active provider (library
).Using a static
web3
can desync from the user’s wallet/provider.Apply:
-import { web3 } from '../../bootstrap/dapp-api';
10-10
: Plumblibrary
from Web3React.Needed to call
eth.getCode
on the active provider.- const { account } = useWeb3React(); + const { account, library } = useWeb3React();
40-49
: Harden the effect: requirelibrary
, add it to deps.Prevents calls before provider is ready and reacts to provider changes.
- useEffect(() => { - if (!account) { + useEffect(() => { + if (!account || !library) { setIsSmartContractWallet(false) return; } updateAccountWarningDismissalState(account); checkIfSmartContractWallet(account); - }, [account]); + }, [account, library]);
🧹 Nitpick comments (2)
src/components/sc-wallet-warning/index.js (2)
14-25
: Normalize storage key by address casing and stringify booleans; wrap setItem in try/catch.Avoid duplicate keys due to checksum casing and guard against storage errors.
const updateAccountWarningDismissalState = (account) => { try { - const storedValue = localStorage.getItem(`${STORAGE_KEY}:${account}`) + const key = `${STORAGE_KEY}:${String(account).toLowerCase()}` + const storedValue = localStorage.getItem(key) if (storedValue === null) { setShowWarning(true) } else { setShowWarning(JSON.parse(storedValue)) } } catch { setShowWarning(true) } } @@ const handleClose = () => { setShowWarning(false); - localStorage.setItem(`${STORAGE_KEY}:${account}`, false); + const key = `${STORAGE_KEY}:${String(account).toLowerCase()}` + try { + localStorage.setItem(key, JSON.stringify(false)); + } catch {} }Also applies to: 54-57
59-73
: A11y: add role/aria-live and a labeled close button.Improves screen-reader UX and prevents form-submit side effects.
- <div className="sc-wallet-warning"> + <div className="sc-wallet-warning" role="alert" aria-live="polite"> @@ - <button onClick={handleClose}>X</button> + <button type="button" onClick={handleClose} aria-label="Dismiss smart contract wallet warning">×</button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/sc-wallet-warning/index.js
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/sc-wallet-warning/index.js (1)
src/bootstrap/dapp-api.js (1)
web3
(12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - escrow
- GitHub Check: Header rules - escrow
- GitHub Check: Pages changed - escrow
🔇 Additional comments (2)
src/components/sc-wallet-warning/index.js (2)
4-4
: Confirmed stylesheet import is correctThe file
src/components/sc-wallet-warning/sc-wallet-warning.css
exists alongside its SCSS source, so the import inindex.js
is valid.
6-6
: EIP-7702 prefix constant confirmed
- Per the EIP-7702 specification, each delegation indicator is defined as the fixed byte sequence
0xef0100
followed by the 20-byte target address (0xef0100 || address
) (eips.ethereum.org).- In practice, calls to
eth_getCode
on upgraded EOAs consistently return values beginning with0xef0100
, confirming this prefix is both canonical and stable (netbasal.medium.com).No changes needed—
0xef0100
is the correct discriminator for EOA-with-7702 code.
|
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
♻️ Duplicate comments (2)
src/components/sc-wallet-warning/index.js (2)
39-47
: Harden the effect against races and RPC failures.Guarding for missing
account/library
is good. Consider cancellation to avoid setState after unmount/account switch, and add a.catch
path surfaced to the user (e.g., log once or telemetry). No diff provided to keep the change minimal with your current structure.
26-37
: Fix SC wallet detection: normalize code, handle empty variants, and support both ethers/web3 providers.Currently compares against raw
code
and only checks'0x'
, which can misclassify ('0x0'
,'0x00'
). Also hard-codes web3.js API. Refactor as below.- const checkIfSmartContractWallet = (account, library) => { - library.eth.getCode(account).then((code) => { - const formattedCode = code.toLowerCase() - const isEip7702Eoa = formattedCode.startsWith(EIP7702_PREFIX) - - //Do not show warning for EIP-7702 EOAs - setIsSmartContractWallet(code !== '0x' && !isEip7702Eoa) - }).catch((error) => { - console.error('Error checking smart contract wallet', error) - setIsSmartContractWallet(false) - }); - } + const checkIfSmartContractWallet = (account, library) => { + if (!account || !library) { + setIsSmartContractWallet(false); + return; + } + const getCode = + typeof library?.getCode === 'function' + ? library.getCode.bind(library) // ethers.js + : library?.eth?.getCode?.bind(library.eth); // web3.js + if (!getCode) { + console.warn('Provider does not support getCode'); + setIsSmartContractWallet(false); + return; + } + getCode(account) + .then((code) => { + const normalized = ((code ?? '0x') + '').toLowerCase(); + const isEip7702Eoa = normalized.startsWith(EIP7702_PREFIX); + const isEmpty = normalized === '0x' || normalized === '0x0' || normalized === '0x00'; + // Do not show warning for EOAs or EIP-7702-authorized EOAs + setIsSmartContractWallet(!isEmpty && !isEip7702Eoa); + }) + .catch((error) => { + console.error('Error checking smart contract wallet', error); + setIsSmartContractWallet(false); + }); + }
🧹 Nitpick comments (2)
src/components/sc-wallet-warning/index.js (2)
53-56
: Persist a stringified boolean and guard storage errors.
localStorage.setItem
coerces to string implicitly; useJSON.stringify(false)
for consistency with the read path and wrap in try/catch.const handleClose = () => { setShowWarning(false); - localStorage.setItem(`${STORAGE_KEY}:${account}`, false); + try { + if (account) { + localStorage.setItem(`${STORAGE_KEY}:${account}`, JSON.stringify(false)); + } + } catch { + // ignore storage errors + } }
58-71
: Improve accessibility of the banner and close button.Add
role="alert"
andaria-live
, and label the close button for screen readers.- <div className="sc-wallet-warning"> + <div className="sc-wallet-warning" role="alert" aria-live="polite"> <p> You are using a smart contract wallet. This is not recommended.{" "} <a href="https://docs.kleros.io/kleros-faq#can-i-use-a-smart-contract-account-to-stake-in-the-court" target="_blank" rel="noopener noreferrer" > Learn more. </a> </p> - <button onClick={handleClose}>X</button> + <button type="button" onClick={handleClose} aria-label="Dismiss smart contract wallet warning">×</button> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/sc-wallet-warning/index.js
(1 hunks)
🔇 Additional comments (3)
src/components/sc-wallet-warning/index.js (3)
13-24
: LocalStorage read path looks solid.Null-safe read + JSON.parse with try/catch is correct for booleans.
49-51
: Render guard is correct.Early-return when hidden or not a SC wallet keeps the DOM clean.
9-10
: Smart contract wallet detection is correct with the current Web3 providerThe application’s
getLibrary
implementation insrc/bootstrap/app.js
explicitly returns a Web3.js instance (new Web3(provider)
), solibrary.eth.getCode
is defined and will work as expected. No changes to the SC-wallet warning component are needed at this time.– You can remove the verification note; if the provider strategy ever changes to Ethers.js in the future, the detection logic should be revisited.
PR-Codex overview
This PR introduces a new component called
SmartContractWalletWarning
to alert users when they are using a smart contract wallet. It includes styling for the warning and logic to determine if the connected wallet is a smart contract wallet.Detailed summary
SmartContractWalletWarning
component insrc/components/sc-wallet-warning/index.js
.src/components/sc-wallet-warning/sc-wallet-warning.scss
.src/bootstrap/app.js
.Summary by CodeRabbit
New Features
Style