Chore/safety capabilities #213
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughÓrale, aquí te va: Se añadieron nuevas funciones y lógica pa’ soportar carteras multisig (multifirma) en el flujo de transacciones. Ahora, varias funciones detectan si el usuario está en un entorno multisig y, si sí, usan el Safe Apps SDK pa’ mandar transacciones en vez de hacerlo directo. También se actualizó el changelog, el package y se agregó un nuevo módulo utilitario pa’ Safe. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant SafeSDK
participant Contract
User->>App: Initiate transaction (approve/send)
App->>SafeSDK: detectMultisig()
SafeSDK-->>App: true/false
alt Multisig detected
App->>SafeSDK: sendViaSafe({to, abi, functionName, args})
SafeSDK->>Contract: execute transaction via Safe
Contract-->>SafeSDK: result
SafeSDK-->>App: {success: true}
else Not multisig
App->>Contract: writeContract({to, abi, functionName, args})
Contract-->>App: result
end
App-->>User: Transaction status/result
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/hooks/useTransactionHelpers.ts (1)
142-155: 🛠️ Refactor suggestionChequeo de allowance anda adelantado pa’ multisig – se va a quedar esperando
Con Safe la aprobación queda pendiente hasta que los owners firmen y ejecuten la tx; llamarcheckAllowanceinmediatamente va a fallar (el allowance sigue igual). Bríncalo o agrega polling para multisig:- const approveResult = await approveERC20(/* … */); - if (!approveResult) { - throw new Error("Failed to approve tokens"); - } - - updateStep({ message: "form.status.validateAllowance", type: "loading" }); - - await checkAllowance(/* … */); + const approveResult = await approveERC20(/* … */); + if (!approveResult) { + throw new Error("Failed to approve tokens"); + } + + if (!isMultisig) { + updateStep({ message: "form.status.validateAllowance", type: "loading" }); + await checkAllowance(/* … */); + updateStep({ + message: "form.status.validateAllowanceCompleted", + type: "completed", + }); + }Así evitas bloqueos falsos cuando la wallet es multisig.
🧹 Nitpick comments (3)
CHANGELOG.md (1)
5-8: Need to fix el formato de los headings, carnalLos headings en el CHANGELOG deberían seguir una estructura jerárquica correcta. Según markdownlint, los headings solo deberían incrementar en un nivel a la vez. Deberías usar
##en lugar de###para mantener la jerarquía adecuada.También estaría firme agregar una descripción breve de los cambios relacionados con el soporte multisig.
-### [0.1.20-beta.1](https://github.com/bandohq/widget/compare/v0.1.20-beta.0...v0.1.20-beta.1) (2025-05-09) +## [0.1.20-beta.1](https://github.com/bandohq/widget/compare/v0.1.20-beta.0...v0.1.20-beta.1) (2025-05-09) + +### Features + +* Added Safe Apps SDK integration for multisig support -### [0.1.20-beta.0](https://github.com/bandohq/widget/compare/v0.1.19...v0.1.20-beta.0) (2025-05-09) +## [0.1.20-beta.0](https://github.com/bandohq/widget/compare/v0.1.19...v0.1.20-beta.0) (2025-05-09) + +### Features + +* Initial implementation of multisig wallet support🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
src/utils/safeFunctions.ts (1)
18-30: La función sendViaSafe funciona bien pero podría tener mejor typingLa función hace su jale correctamente, pero podría beneficiarse de types explícitos de TypeScript para evitar errores. También recomendaría agregar manejo de errores más específico.
-export const sendViaSafe = async ({ to, abi, functionName, args }) => { +interface SafeTxParams { + to: string; + abi: any[]; + functionName: string; + args: any[]; +} + +export const sendViaSafe = async ({ to, abi, functionName, args }: SafeTxParams) => { const data = encodeFunctionData({ abi, functionName, args, }); - await sdk.txs.send({ - txs: [{ to, value: "0", data }], - }); - - return { success: true }; + try { + await sdk.txs.send({ + txs: [{ to, value: "0", data }], + }); + return { success: true }; + } catch (error) { + console.error("Error sending transaction via Safe:", error); + return { success: false, error }; + } }src/hooks/useTransactionHelpers.ts (1)
32-32: Estás detectando multisig varias veces – cachea el resultado
detectMultisighace una llamada RPC (300 ms timeout). La ejecutas en cada helper; durante un mismo flujo el status no cambia. Guarda el resultado en unuseRefo en contexto para evitar latencia extra.Ejemplo rápido:
const multisigRef = useRef<Promise<boolean>>(); const isMultisig = async () => { if (!multisigRef.current) multisigRef.current = detectMultisig(); return multisigRef.current; };Also applies to: 71-71, 129-129
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)package.json(2 hunks)src/hooks/useTransactionHelpers.ts(9 hunks)src/utils/checkAllowance.ts(2 hunks)src/utils/safeFunctions.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/utils/checkAllowance.ts (2)
src/utils/safeFunctions.ts (2)
detectMultisig(6-16)sdk(4-4)src/utils/abis.ts (1)
ERC20AllowanceABI(13-24)
src/hooks/useTransactionHelpers.ts (2)
src/utils/safeFunctions.ts (3)
detectMultisig(6-16)sendViaSafe(18-30)sdk(4-4)src/utils/abis.ts (1)
ERC20ApproveABI(26-37)
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
5-5: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (6)
package.json (2)
3-3: Veo que bumpearon la versión pa' un beta release, ¡qué chido!Esta versión beta refleja la adición de las nuevas multisig capabilities. Todo looks good.
54-54: ¡Firme! Agregaste el Safe Apps SDK pa' los multisig walletsLa adición del Safe Global SDK es crucial para implementar el soporte de carteras multifirma que se ha introducido en este PR.
src/utils/checkAllowance.ts (2)
3-3: Órale, buena onda importando las multisig functionsLa integración del módulo de Safe y la detección de multisig está bien implementada aquí. Detectas el multisig environment una sola vez antes del loop, lo cual es más eficiente que hacerlo en cada intento.
Also applies to: 17-17
23-40: La lógica condicional para multisig está al cien, eseEl código maneja correctamente ambos escenarios:
- Para wallets multisig, usa la safe address como owner
- Para wallets regulares, usa la account address como antes
Este approach es sólido y permite que el mismo código funcione en ambos contextos sin problemas.
src/utils/safeFunctions.ts (2)
1-4: ¡Qué chingón! Nuevo módulo pa' Safe Apps SDKLa inicialización del SDK está bien implementada como un singleton que se puede reutilizar en toda la app.
6-16: La función detectMultisig es bien smart, carnalitoMe gusta cómo usas Promise.race con un timeout para evitar que se quede hanging cuando no estás en un Safe environment. El timeout de 300ms es un buen balance entre esperar lo suficiente y no afectar la experiencia del usuario.
Summary by CodeRabbit