Feature/social login sdk integration#212
Feature/social login sdk integration#212AnoukRImola wants to merge 8 commits intoStellar-Rent:mainfrom
Conversation
📝 WalkthroughWalkthroughReplaces backend wallet-auth route with a client-side Stellar Social SDK and adds a full social-auth SDK, Google/Freighter providers, frontend auth flows, new auth UI, atomic DB RPCs for sync/events and payments, enriched backend error types/middleware, and extensive SDK and backend/service changes. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser as Browser/Client
participant GoogleAPI as Google Identity Services
participant SDK as Stellar Social SDK
participant Horizon as Stellar Horizon
participant Backend as Backend API
User->>Browser: Click "Continue with Google"
Browser->>GoogleAPI: Render Google button / authenticate
GoogleAPI-->>Browser: Return credential token
Browser->>SDK: loginWithGoogle(credential)
SDK->>SDK: verify token & derive deterministic keypair
SDK->>Horizon: Check account existence
alt Account exists
Horizon-->>SDK: Account data
else Create account
SDK->>Horizon: Create/fund account (testnet friendbot)
Horizon-->>SDK: New account created
end
SDK-->>Browser: Session established (user, authMethod, account)
Browser->>Backend: Fetch user/profile by publicKey
Backend-->>Browser: Profile data
Browser->>Browser: Navigate to /dashboard
sequenceDiagram
actor User
participant Browser as Browser/Client
participant FreighterExt as Freighter Wallet
participant SDK as Stellar Social SDK
participant Horizon as Stellar Horizon
participant Backend as Backend API
User->>Browser: Click "Connect with Freighter"
Browser->>FreighterExt: Check installation
alt Not installed
FreighterExt-->>Browser: Not installed -> show install link
else Installed
Browser->>FreighterExt: Request access
FreighterExt-->>Browser: Return publicKey
Browser->>SDK: loginWithFreighter()
SDK->>Horizon: Load or create account for publicKey
SDK-->>Browser: Session established
Browser->>Backend: Fetch profile by publicKey
Backend-->>Browser: Profile data
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/hooks/useUserRole.tsx (1)
33-80: Scope cached host status to the current user.
Global localStorage keys can leak a previous user’s host status if the API fails, resulting in incorrect roles/permissions UI. Use user-scoped keys (e.g., by publicKey) or clear on logout.🐛 Suggested fix
- try { - const userId = user.publicKey || 'unknown'; - const response = await profileAPI.getUserProfile(userId); + const userId = user.publicKey || 'unknown'; + const hostStatusKey = `hostStatus:${userId}`; + const hasPropertiesKey = `hasProperties:${userId}`; + + try { + const response = await profileAPI.getUserProfile(userId); const profile = response.data; @@ - if (hostStatus) { - localStorage.setItem('hostStatus', hostStatus); - } - localStorage.setItem('hasProperties', String(hasProperties)); + if (hostStatus) { + localStorage.setItem(hostStatusKey, hostStatus); + } + localStorage.setItem(hasPropertiesKey, String(hasProperties)); } catch (apiError) { @@ - const storedHostStatus = localStorage.getItem('hostStatus'); - const storedHasProperties = localStorage.getItem('hasProperties') === 'true'; + const storedHostStatus = localStorage.getItem(hostStatusKey); + const storedHasProperties = localStorage.getItem(hasPropertiesKey) === 'true';
🤖 Fix all issues with AI agents
In `@apps/web/package.json`:
- Line 15: The local package dependency for `@stellar/stellar-sdk` in the
stellar-social-sdk package.json is pinned to ^12.3.0 while the parent app uses
^13.3.0; update the dependency version in stellar-social-sdk's package.json to
"^13.3.0", then reinstall and regenerate the lockfile (npm/yarn/pnpm) so the
workspace resolves a single `@stellar/stellar-sdk` version; after upgrading, run
the stellar-social-sdk build/tests and address any API breakages in
functions/classes that import `@stellar/stellar-sdk`.
In `@apps/web/src/app/login/page.tsx`:
- Around line 13-20: The redirect query value pulled from
searchParams.get('redirect') is used unvalidated in router.push, allowing
open-redirects; update the logic around const redirect and the useEffect so you
first validate the value (e.g. ensure it is an internal path that starts with
'/' but not '//' and contains no scheme like 'http:' or host), and if the check
fails fall back to '/dashboard'; then use the validatedRedirect variable in the
useEffect where router.push is called (references: searchParams.get, redirect,
useEffect, router.push, isAuthenticated, isLoading).
In `@apps/web/src/components/auth/FreighterFallback.tsx`:
- Around line 27-31: The code treats Freighter as installed by checking property
existence on the result of isConnected() instead of its boolean value; update
the assignment that uses the connected variable so setIsFreighterInstalled
receives the actual boolean (connected.isConnected) rather than doing an
existence check, e.g. change the expression in the setIsFreighterInstalled call
to use connected.isConnected (or safely coerce it to a boolean) so false is
respected; refer to the isConnected() call and the setIsFreighterInstalled
invocation to locate the change.
In `@apps/web/src/hooks/auth/use-auth.tsx`:
- Around line 96-121: The session restore currently in the useEffect
(restoreSession) sets user and authMethod from STORAGE_KEYS but never rehydrates
account, causing isAuthenticated to be true while account is null; update the
hook to, after setting user/authMethod, attempt to rehydrate account via the SDK
(wait for sdk readiness), e.g. call sdk.loadExistingAccount(user.publicKey) (or
equivalent) and setAccount(restored) when successful, and on failure call
clearStorage(), setUser(null), setAuthMethod(null); also ensure isAuthenticated
is gated on account rather than only on user/authMethod so UI won’t show a
logged-in state until account is restored.
In `@apps/web/stellar-social-sdk/package.json`:
- Around line 17-23: The project currently depends on crypto-js and uses
CryptoUtils.generateKeypair() to derive Ed25519 seeds; replace the JS-based
CryptoJS.SHA256 usage with the Web Crypto API by converting the input to an
ArrayBuffer and calling crypto.subtle.digest('SHA-256', ...) and using the
resulting hash as the deterministic seed in CryptoUtils.generateKeypair(),
removing the crypto-js dependency from package.json; also move
"@stellar/stellar-sdk" and "@stellar/freighter-api" from dependencies into
peerDependencies in package.json so consuming apps control the SDK versions and
avoid duplicate installs.
In `@apps/web/stellar-social-sdk/src/index.ts`:
- Around line 64-70: This code logs user PII (authMethod.metadata?.name and
authMethod.metadata?.email) to the client console; remove those console.log
calls or redact/gate them behind a debug flag. Locate the block using
CryptoUtils.generateKeypair('google', googleSub) and keypair.publicKey(), keep
the public key log if needed, but delete or replace the
authMethod.metadata?.name/email console.log with either a non-PII message,
masked values, or wrap it in a runtime DEBUG check so it only runs in
development builds.
- Around line 215-238: The current catch blocks in getOrCreateAccount,
getOrCreateAccountWithKeypair, and loadExistingAccount treat any loadAccount
failure as "account missing" and always call createNewAccount; change each catch
to inspect the thrown error and only proceed to createNewAccount when the
Horizon response indicates a 404 (e.g., error.response?.status === 404 or
error.status === 404 or error is StellarSdk.NotFoundError), otherwise rethrow
the error so network/rate-limit/other Horizon errors bubble up; specifically
update the catch around this.server.loadAccount(...) in getOrCreateAccount,
getOrCreateAccountWithKeypair, and loadExistingAccount to conditionally call
createNewAccount(keypair, authMethod) only on 404 and throw the original error
in all other cases.
In `@apps/web/stellar-social-sdk/src/index.ts.corrupted`:
- Around line 124-154: In authenticateWithPhone enforce that the hardcoded test
verification code ('123456') is only accepted when a development/test flag is
enabled: check an environment flag (e.g. process.env.NODE_ENV === 'development'
or a dedicated flag like process.env.ALLOW_TEST_PHONE_CODE) before allowing that
bypass; otherwise treat '123456' as invalid and require real verification; add a
clear warning log when the test code path is used and keep existing error
handling intact (references: authenticateWithPhone, CryptoUtils.hashPhone,
getOrCreateAccount).
- Around line 88-119: The current authenticateWithFacebook method in the class
returns nondeterministic mock accounts (uses Math.random()), which breaks
deterministic account behavior; replace the implementation with an immediate
not-implemented error to prevent accidental use: update
authenticateWithFacebook() to synchronously/async-throw a clear Error (e.g.
"authenticateWithFacebook is not implemented") or return a rejected Promise, and
remove any calls that create random AuthMethod/getOrCreateAccount flow;
reference the authenticateWithFacebook function and avoid constructing
AuthMethod or calling getOrCreateAccount so callers cannot obtain random
wallets.
- Around line 314-390: The two methods getOrCreateAccountWithKeypair and
createNewAccountWithKeypair are defined outside the StellarSocialSDK class
(causing a syntax error); move both method definitions into the StellarSocialSDK
class body (e.g., before the export statements) preserving their signatures,
async/private modifiers and usage of this.server/this.contractId/this.network,
then remove the out-of-class duplicates so only the class contains these methods
and exports remain after the class definition.
In `@apps/web/stellar-social-sdk/src/providers/GoogleAuthProvider.ts`:
- Around line 172-185: The decodeJWT function fails for base64url payloads
missing '=' padding; update decodeJWT to normalize the payload by replacing
URL-safe characters (replace '-' -> '+' and '_' -> '/'), then append '='
characters until payload.length % 4 === 0 before calling atob, and keep the
existing JSON.parse and error handling flow so the JWT payload decodes reliably.
- Around line 136-167: The verifyToken implementation currently calls decodeJWT
without checking the signature; update verifyToken to perform JWKS-based
signature verification: fetch Google's JWKS from
https://www.googleapis.com/oauth2/v3/certs, select the key matching the token's
header.kid, convert that JWKS entry to a usable public key, verify the JWT
signature before trusting the payload returned by decodeJWT, and then enforce
claim checks (iss === 'https://accounts.google.com' or 'accounts.google.com',
aud === this.clientId, exp > now, and presence of sub and email). Use
verifyToken and decodeJWT names to locate the logic and ensure errors surface
descriptive messages on verification failure; if this code must remain in the
frontend, move signature verification into a backend endpoint instead.
In `@apps/web/stellar-social-sdk/src/utils/crypto.ts`:
- Around line 45-52: The hashPhone method currently in utils/crypto.ts (static
hashPhone) is vulnerable to brute-force because it hashes low-entropy phone
numbers and then truncates the result; change it to combine the cleaned phone
number with a secret pepper (server-side) and run it through a computational KDF
(e.g., PBKDF2 or scrypt) with a per-item salt and sufficient iterations/work
factor, then output the full derived value (avoid arbitrary truncation). Ensure
the secret pepper is not stored in client code, and update any consumers of
hashPhone to expect the KDF output format (salt + derived key) rather than the
current truncated hex.
🟡 Minor comments (10)
apps/backend/src/_deprecated/README.md-31-33 (1)
31-33: Use a precise migration date.“2024” is ambiguous; please update this to an exact YYYY‑MM‑DD date once confirmed so the deprecation timeline is unambiguous.
apps/backend/src/_deprecated/README.md-3-16 (1)
3-16: Align SDK naming and provider list with actual support.The doc alternates between “Stellar Social SDK” and “Stellar Account Abstraction SDK,” and it claims Facebook/Phone support even though the PR objectives say Google-only. Please make the naming consistent and mark non‑Google providers as planned if they’re not live yet.
✍️ Proposed doc tweak
-These files were deprecated as part of the migration to **Stellar Social SDK** for authentication. +These files were deprecated as part of the migration to **Stellar Account Abstraction (Social) SDK** for authentication. -This has been replaced with the **Stellar Account Abstraction SDK** which: +This has been replaced with the **Stellar Account Abstraction (Social) SDK** which: - Handles authentication entirely on the client-side -- Supports social login (Google, Facebook, Phone) +- Supports social login (Google; Facebook/Phone planned)apps/backend/src/index.ts-66-69 (1)
66-69: Remove or update stale wallet-auth tests that reference non-existent endpoints.The wallet-auth endpoints (
/api/auth/challenge,/api/auth/wallet) have already been removed and are not mounted inindex.ts. However,apps/backend/tests/integration/wallet-auth.test.tsstill contains tests that attempt to call these removed endpoints. These tests should either be deleted (if no longer needed) or moved to a deprecated test suite. Additionally, web tests inapps/web/tests/e2e/register.spec.tsmock/api/auth/register, but the actual mounted endpoint is/auth/register—verify test paths align with current route definitions.apps/web/stellar-social-sdk/package.json.backup-1-16 (1)
1-16: Remove backup file from version control.Backup files (
.backup) should not be committed. Use.gitignoreto exclude them, or delete this file if it's no longer needed. The actualpackage.jsonshould be the source of truth.Additionally, the author field contains a placeholder
"tu-nombre"that should be updated.apps/web/src/app/layout.tsx-23-26 (1)
23-26: Permissions-Policy viahttpEquivmeta tag is ineffective.The
Permissions-Policyheader cannot be set via a<meta http-equiv>tag—browsers ignore it. This policy must be delivered as an HTTP response header.If you need to restrict these permissions, configure your server or Next.js middleware to return the header:
// middleware.ts or next.config.js headers() { key: 'Permissions-Policy', value: 'identity-credentials-get=(), publickey-credentials-get=()' }apps/web/stellar-social-sdk/src/utils/crypto.ts-8-27 (1)
8-27: Security consideration: deterministic key derivation from social identity using custom KDF.This implementation derives a Stellar keypair deterministically from provider ID + hardcoded salt using a custom SHA256-based scheme. This approach diverges from Stellar's recommended SEP-0005 standard (which uses BIP-39 + SLIP-0010 hardened derivation) and introduces specific risks:
- Reproducibility risk: Anyone with the user's provider ID (e.g., Google
sub) and knowledge of the hardcoded saltstellar-social-v1can derive the same keypair. If the provider ID is discoverable through normal account flows or exposed in logs/errors, the wallet keypair is compromised.- Non-standard KDF: Using ad-hoc hashing for key material differs from established key derivation standards. Consider adopting SEP-0005 or documenting why a custom approach is necessary.
This is a deliberate trade-off for seedless wallet UX. Mitigate by:
- Ensuring provider IDs are never logged, exposed in errors, or accessible to untrusted parties
- Documenting this security model so users and operators understand the trust assumptions
- Considering whether the salt should be per-user or environment-specific rather than hardcoded globally
apps/web/stellar-social-sdk/package.json-12-12 (1)
12-12: Test script references non-existentdist/test.js.The test script runs
node dist/test.js, but the build configuration (Rollup) only generatesindex.js,index.esm.js, andindex.d.ts. This script will fail unlesstest.jsis explicitly built or the script is updated.Consider either:
- Adding a proper test framework (e.g., vitest, jest)
- Updating the script to reference the correct test file location
apps/web/src/components/auth/SocialLoginButton.tsx-16-105 (1)
16-105: Clear retry timers on unmount to avoid post‑unmount state updates.The retry
setTimeouts aren’t cleared; if the component unmounts during navigation, callbacks can still fire and callsetState/handleError. Add a cancel flag + cleanup to prevent leaks and React warnings.🧹 Proposed fix
const [state, setState] = useState<ButtonState>('loading'); const [errorMessage, setErrorMessage] = useState<string | null>(null); const buttonContainerRef = useRef<HTMLDivElement>(null); const renderAttempts = useRef(0); const hasRendered = useRef(false); + const timeoutIds = useRef<number[]>([]); const maxAttempts = 20; // 10 seconds max wait useEffect(() => { + let cancelled = false; + const schedule = (fn: () => void, ms: number) => { + const id = window.setTimeout(() => { + if (!cancelled) fn(); + }, ms); + timeoutIds.current.push(id); + }; + if (!GOOGLE_CLIENT_ID) { handleError('Google Client ID not configured'); return; } const renderGoogleButton = () => { + if (cancelled) return; // Wait for the ref to be available if (!buttonContainerRef.current) { renderAttempts.current += 1; if (renderAttempts.current < maxAttempts) { - setTimeout(renderGoogleButton, 100); + schedule(renderGoogleButton, 100); } else { handleError('Button container not available'); } return; } @@ } else { // Retry if script hasn't loaded yet renderAttempts.current += 1; if (renderAttempts.current < maxAttempts) { - setTimeout(renderGoogleButton, 500); + schedule(renderGoogleButton, 500); } else { handleError('Google Sign-In unavailable'); } } }; // Start trying to render immediately renderGoogleButton(); + return () => { + cancelled = true; + timeoutIds.current.forEach(clearTimeout); + timeoutIds.current = []; + }; }, [handleError]);apps/web/stellar-social-sdk/src/auth/StellarSocialAccount.ts-100-107 (1)
100-107: Handle non‑standard asset types in balances.Horizon returns multiple asset types in account balances, including
liquidity_pool_shares(Protocol 18+). Theliquidity_pool_sharestype does not includeasset_codeorasset_issuerfields—instead it hasliquidity_pool_id—but your code assumes these fields always exist for non-native types. This producesundefined:undefinedfor pool share balances.Handle each asset type explicitly:
native→'XLM',liquidity_pool_shares→'LP:{liquidity_pool_id}', standard credit types (credit_alphanum4/12) →'{asset_code}:{asset_issuer}', and add a fallback for any future types.Suggested fix
- return account.balances.map((balance: any) => ({ - balance: balance.balance, - asset: balance.asset_type === 'native' ? 'XLM' : - `${balance.asset_code}:${balance.asset_issuer}` - })); + return account.balances.map((balance: any) => { + if (balance.asset_type === 'native') { + return { balance: balance.balance, asset: 'XLM' }; + } + if (balance.asset_type === 'liquidity_pool_shares') { + return { balance: balance.balance, asset: `LP:${balance.liquidity_pool_id}` }; + } + if (balance.asset_code && balance.asset_issuer) { + return { balance: balance.balance, asset: `${balance.asset_code}:${balance.asset_issuer}` }; + } + return { balance: balance.balance, asset: balance.asset_type }; + });apps/web/stellar-social-sdk/src/auth/StellarSocialAccount.ts-44-84 (1)
44-84: Truncate memo by byte length, not character length.Stellar text memos are limited to 28 bytes in the protocol and the SDK enforces this via
Memo._validateTextValue(). The current code usesmemo.length > 28, which counts UTF-16 code units instead of bytes. Non-ASCII UTF-8 characters (e.g., emojis, accented characters) can encode to multiple bytes, causing the string to exceed 28 bytes even when the character count is under the limit. This will fail when submitted to the network.Use
TextEncoder.encodeInto()to safely truncate at byte boundaries without splitting multibyte UTF-8 sequences:🧭 Proposed fix
if (memo) { // Stellar text memos have a 28-byte limit - const truncatedMemo = memo.length > 28 ? memo.substring(0, 28) : memo; + const encoder = new TextEncoder(); + const buffer = new Uint8Array(28); + const { read } = encoder.encodeInto(memo, buffer); + const truncatedMemo = memo.slice(0, read); txBuilder.addMemo(Memo.text(truncatedMemo)); }
🧹 Nitpick comments (13)
apps/web/stellar-social-sdk/src/utils/crypto.ts (1)
37-43:isValidJWTprovides only structural validation, not security validation.This method checks only that the token has three dot-separated parts. It does not verify the signature, expiration, or issuer. Consider renaming to
hasJWTStructureor adding a clear docstring warning that this is insufficient for auth validation.Suggested rename for clarity
/** - * Verify JWT token structure (basic validation) + * Check if string has JWT structure (3 dot-separated parts). + * WARNING: Does NOT verify signature, expiry, or claims. */ - static isValidJWT(token: string): boolean { + static hasJWTStructure(token: string): boolean { const parts = token.split('.'); return parts.length === 3; }apps/web/stellar-social-sdk/src/index.ts.corrupted (2)
237-269: Duplicate logic:createNewAccountandcreateNewAccountWithKeypairare nearly identical.These two methods differ only in how the keypair is passed to
StellarSocialAccount. Consider consolidating into a single method that always accepts a keypair parameter.Also applies to: 358-390
247-247: Fixed 3-second delay may be insufficient or excessive.The hardcoded
setTimeout(resolve, 3000)after Friendbot funding is arbitrary. Network conditions vary, and this could either fail (if funding takes longer) or unnecessarily slow the UX.Consider polling the account existence instead:
// Poll until account exists or timeout const maxAttempts = 10; for (let i = 0; i < maxAttempts; i++) { try { await this.server.loadAccount(publicKey); break; } catch { await new Promise(r => setTimeout(r, 1000)); } }Also applies to: 367-368
apps/web/src/lib/stellar-social-sdk.ts (1)
1-10: Use thestellar-social-sdkpath alias instead of a relative path.Since
tsconfig.jsonalready defines a path alias forstellar-social-sdk, consider using it here for consistency and easier maintenance:-export { StellarSocialSDK } from '../../stellar-social-sdk/dist/index.esm.js'; +export { StellarSocialSDK } from 'stellar-social-sdk'; export type { SocialAuthConfig, AuthMethod, AuthResult, SocialAccountData, -} from '../../stellar-social-sdk/dist/index.esm.js'; +} from 'stellar-social-sdk';Also, consider using English for comments to maintain consistency with the rest of the codebase.
apps/web/src/app/register/page.tsx (1)
12-27: Consider using a server-side redirect innext.config.jsinstead.A client-side redirect requires JavaScript execution and shows a loading spinner. For better UX and SEO, consider using Next.js permanent redirects in
next.config.js:// next.config.js module.exports = { async redirects() { return [ { source: '/register', destination: '/login', permanent: true, // 308 redirect }, ]; }, };This eliminates the need for this page entirely, provides instant redirection, and works without JavaScript.
apps/web/stellar-social-sdk/tsconfig.json (1)
1-17: LGTM with a minor suggestion onmoduleResolution.The configuration is appropriate for building the SDK. Consider updating
moduleResolutionto"bundler"or"node16"for better ESM support and consistency with the parent app's tsconfig:- "moduleResolution": "node" + "moduleResolution": "bundler"The
"node"resolution mode is the legacy CommonJS resolution algorithm and may behave differently than the parent app's"bundler"mode.apps/web/stellar-social-sdk/src/config.ts (1)
1-3: Clarify that the default contract ID is testnet-specific.Since the default appears tied to testnet, consider renaming to
DEFAULT_TESTNET_CONTRACT_ID(or removing the default entirely) to avoid accidental mainnet usage without an explicit contract ID.apps/web/src/components/layout/Navbar.tsx (1)
51-53: Avoid “undefined...” in the user name fallback.If
user.nameis empty anduser.publicKeyis missing, the UI can render “undefined...”. Consider a safer fallback.💡 Suggested change
- <span className="hidden sm:inline text-muted-foreground"> - {user.name || `${user.publicKey?.slice(0, 8)}...`} - </span> + <span className="hidden sm:inline text-muted-foreground"> + {user.name ?? (user.publicKey ? `${user.publicKey.slice(0, 8)}...` : 'User')} + </span>apps/web/src/hooks/auth/__tests__/use-auth.test.tsx (1)
67-75: Reset NEXT_PUBLIC_ env vars after each test run.*
They’re overwritten inbeforeEachbut never restored, which can leak into other test files with different env expectations.♻️ Suggested fix
// Wrapper para el provider const wrapper = ({ children }: { children: ReactNode }) => <AuthProvider>{children}</AuthProvider>; + +const ORIGINAL_ENV = { + NEXT_PUBLIC_GOOGLE_CLIENT_ID: process.env.NEXT_PUBLIC_GOOGLE_CLIENT_ID, + NEXT_PUBLIC_CONTRACT_ID: process.env.NEXT_PUBLIC_CONTRACT_ID, + NEXT_PUBLIC_STELLAR_NETWORK: process.env.NEXT_PUBLIC_STELLAR_NETWORK, +}; + +const restoreEnvVar = (key: keyof typeof ORIGINAL_ENV) => { + const value = ORIGINAL_ENV[key]; + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } +}; describe('useAuth Hook', () => { beforeEach(() => { jest.clearAllMocks(); localStorageMock.clear(); // Reset environment variables process.env.NEXT_PUBLIC_GOOGLE_CLIENT_ID = 'test-client-id'; process.env.NEXT_PUBLIC_CONTRACT_ID = 'test-contract-id'; process.env.NEXT_PUBLIC_STELLAR_NETWORK = 'testnet'; }); + + afterEach(() => { + restoreEnvVar('NEXT_PUBLIC_GOOGLE_CLIENT_ID'); + restoreEnvVar('NEXT_PUBLIC_CONTRACT_ID'); + restoreEnvVar('NEXT_PUBLIC_STELLAR_NETWORK'); + });apps/web/stellar-social-sdk/src/providers/FreighterProvider.ts (1)
33-40: Add fallback forpublicKeyfield when normalizingrequestAccess()response.The code currently handles
stringand{ address }, but the local type definition in the codebase declares{ publicKey }. While@stellar/freighter-apiv4.1.0 appears to useaddress, adding a fallback improves defensive compatibility in case of API variations.♻️ Suggested fix
- const publicKey = typeof accessResult === 'string' - ? accessResult - : accessResult.address; + const publicKey = + typeof accessResult === 'string' + ? accessResult + : accessResult.address ?? accessResult.publicKey;apps/web/src/hooks/auth/procted-route.tsx (1)
12-19: Stabilize defaultallowedAuthTypesto avoid effect churn.The default array literal is recreated each render, so the effect dependency changes every time. Define a stable constant to prevent unnecessary re-runs/redirect checks.
♻️ Proposed fix
+const DEFAULT_ALLOWED_AUTH_TYPES = ['google', 'freighter'] as const; + export default function ProtectedRoute({ children, redirectTo = '/login', - allowedAuthTypes = ['google', 'freighter'], + allowedAuthTypes = DEFAULT_ALLOWED_AUTH_TYPES, }: ProtectedRouteProps) {Also applies to: 39-39
apps/web/src/types/auth.ts (1)
14-52: Use a sharedAuthMethodTypeunion to avoid drift.
AuthMethodData.typeisstringwhileSocialUser.authMethodandAuthContextType.authMethodare unions. A shared union keeps types aligned and prevents accidental widening.🧩 Proposed fix
+export type AuthMethodType = 'google' | 'freighter' | 'facebook' | 'phone' | 'passkey'; + export interface SocialUser { publicKey: string; name?: string; email?: string; picture?: string; - authMethod: 'google' | 'freighter'; + authMethod: AuthMethodType; } @@ export interface AuthMethodData { - type: string; + type: AuthMethodType; identifier: string; metadata?: { name?: string; email?: string; picture?: string; }; } @@ export interface AuthContextType { @@ - authMethod: 'google' | 'freighter' | null; + authMethod: AuthMethodType | null;apps/web/stellar-social-sdk/src/index.ts.backup (1)
1-299: Remove the.backupsource file to avoid accidental packaging.Leaving a full SDK implementation in a
.backupfile can confuse tooling or accidentally ship stale APIs. Consider deleting it or excluding it from build/package inputs.
respp
left a comment
respp
left a comment
There was a problem hiding this comment.
We need to address 2 troubles before merging: First, the implementation of atomic database transactions to ensure that event logging and state updates occur as a single unit. Second, the system must validate the Stellar transaction ID before processing any event to prevent duplicate records if the listener retries.
Finally, please avoid silent catches just with clg. If an error occurs and it's only printed to the console without being re-thrown or properly logged to a monitoring system, it creates a false sense of security while critical payment syncs might be failing in the background.
Once these points and the CodeRabbit suggestions are addressed, the PR will be approved.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/src/middleware/error.middleware.ts (1)
54-60: TokenExpiredError branch is shadowed by JsonWebTokenError.Because
TokenExpiredErrorsubclassesJsonWebTokenError, the expired-token path won't run. Swap the checks so expired tokens return the correct message.🛠️ Proposed fix
- } else if (err instanceof JsonWebTokenError) { - statusCode = 401; - response.error = 'Invalid token'; - } else if (err instanceof TokenExpiredError) { - statusCode = 401; - response.error = 'Token expired'; - } else if (err instanceof BookingError) { + } else if (err instanceof TokenExpiredError) { + statusCode = 401; + response.error = 'Token expired'; + } else if (err instanceof JsonWebTokenError) { + statusCode = 401; + response.error = 'Invalid token'; + } else if (err instanceof BookingError) {
🤖 Fix all issues with AI agents
In `@apps/backend/database/migrations/00006_add_atomic_functions.sql`:
- Around line 11-23: The SECURITY DEFINER function process_sync_event_atomic
lacks an explicit search_path; add a fixed, trusted schema search_path (the
schema that owns your DB objects, not public) with pg_temp last by adding a SET
search_path clause to the function declaration (or a SET statement at the top of
the function body) so object resolution cannot be hijacked; ensure "public" is
not included and that pg_temp is the final entry in the search_path.
- Around line 71-86: The EXCEPTION block inside process_sync_event_atomic
currently INSERTs into sync_logs then RAISEs, but that insert is rolled back
when the exception aborts the transaction; change the handler to emit a durable
out-of-transaction notification or server log instead: replace the INSERT with
either a RAISE LOG/NOTICE that includes SQLERRM/SQLSTATE/p_event_id/p_event_type
or call pg_notify (e.g., PERFORM pg_notify('sync_errors', json_build_object(...
)::text)) so a separate listener/process can persist the log, or implement an
out-of-transaction commit via a helper (dblink/autonomous transaction) to write
to sync_logs before re-raising; update the EXCEPTION block in
process_sync_event_atomic accordingly.
In `@apps/backend/database/migrations/00007_add_payment_constraints.sql`:
- Around line 36-57: The payment confirmation allows NULL/empty
p_transaction_hash, letting calls bypass uniqueness—add a guard in the same
function/procedure before updating booking status: validate p_transaction_hash
is NOT NULL and not an empty/whitespace string (check p_transaction_hash IS NOT
NULL AND trim(p_transaction_hash) <> ''), and if invalid RETURN
jsonb_build_object with success=false and an error like 'MISSING_TRANSACTION'
(similar shape to existing returns referencing v_booking and
payment_transaction_hash); keep the subsequent duplicate-exists check as-is so
only non-empty hashes reach it.
In `@apps/backend/src/blockchain/trustlessWork.ts`:
- Around line 306-309: The thrown EscrowError in
apps/backend/src/blockchain/trustlessWork.ts uses the code 'CREATE_ESCROW_FAIL'
which mismatches the middleware's expected 'ESCROW_CREATE_FAIL'; update the
throw to use 'ESCROW_CREATE_FAIL' (i.e., change the errorCode argument in the
EscrowError constructor) so EscrowError instantiations in this file align with
the middleware mapping; verify the change where EscrowError is thrown (the catch
block that currently throws new EscrowError('Failed to create escrow',
'CREATE_ESCROW_FAIL', error)) and adjust any related unit tests or callers if
they assert the previous code.
In `@apps/backend/src/services/booking.service.ts`:
- Around line 516-574: Add an explicit union return type to
confirmBookingPayment to make the fallback shape explicit: change the signature
of confirmBookingPayment to return Promise<Booking | { id: string; status:
string; payment_transaction_hash: string }>, or define and use a named type
(e.g., PartialConfirmedBooking) and return that; update callers (notably
booking.controller.ts usage at the endpoint handling the result) to discriminate
the union and handle both full Booking and the minimal fallback object
accordingly.
In `@apps/backend/src/services/cache.service.ts`:
- Around line 78-81: The catch blocks in cache service currently await
loggingService.logBlockchainOperation and loggingService.logBlockchainError
which can throw and break the "should not stop the application" guarantee;
update each cache error catch (the blocks that call
loggingService.logBlockchainOperation and loggingService.logBlockchainError) to
protect logging by wrapping those logging calls in their own try/catch or call
them without awaiting and attach .catch(...) so any logging error is swallowed,
and then continue to return the existing default (e.g., null or fallback) from
the cache method; apply this pattern to all cache-related catch blocks that
invoke logBlockchainOperation/logBlockchainError (the instances around the shown
snippet and the other occurrences mentioned).
In `@apps/backend/src/services/sync.service.ts`:
- Around line 829-841: Guard against a possible null/undefined RPC payload
before accessing data.success in the atomic processing branch: check `if
(!data)` and log/throw or treat as failure (e.g., logBlockchainError and throw a
SyncError) so `data.success` is only read when `data` is truthy; and change the
duplicate-event return shape from `{ success: true, error: 'DUPLICATE_EVENT' }`
to a clearer form such as `{ success: true, skipped: true, reason: 'duplicate'
}` (and update the method signature to reflect the new optional fields like
skipped/reason/syncEventId) so callers aren’t confused by a success=true with an
error string.
🧹 Nitpick comments (3)
apps/backend/src/services/sync.service.ts (3)
333-343: Log correlation is broken on error path.The original
logIdfrom line 311 should be used for error logging to maintain correlation with the operation that failed. Creating a new log entry loses the link to the original operation context.♻️ Suggested fix
} catch (error) { this.failedEvents++; - // Log the error using loggingService for proper error serialization - const errorLog = await loggingService.logBlockchainOperation('pollForEvents', { - lastProcessedBlock: this.lastProcessedBlock, - }); - await loggingService.logBlockchainError(errorLog, { + // Log the error using the original logId for correlation + await loggingService.logBlockchainError(logId, { error, context: 'Failed to poll for blockchain events', + lastProcessedBlock: this.lastProcessedBlock, }); // Re-throw as SyncError to propagate to callers throw new SyncError('Failed to poll for blockchain events', 'POLL_EVENTS_FAIL', error); }Note: This requires moving the
logIddeclaration to a scope accessible in the catch block (e.g., declaring it before the try block).
521-523: Silent catch block may mask logging infrastructure issues.The empty
.catch(() => {})swallows all errors silently. While the intent to prevent cascading errors is valid, consider at minimum logging toconsole.errorso logging infrastructure failures are observable.♻️ Suggested improvement
}); }) - .catch(() => { - // Ignore logging failures to prevent cascading errors + .catch((loggingError) => { + // Log to console as last resort - don't let logging failures cascade + console.error('Failed to log event parsing error:', loggingError); });
631-631: Double type assertion bypasses type checking.The
as unknown as Record<string, unknown>cast is a type safety escape hatch. SinceBlockchainEventDatahas typed fields, consider updatingsyncBookingFromBlockchainto acceptBlockchainEventDatadirectly, or use a type-safe conversion utility.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@biome.json`:
- Around line 11-21: The JSON array in biome.json's ignore list is invalid
because the entry "apps/web/stellar-social-sdk" is missing a trailing comma; fix
by adding a comma after the "apps/web/stellar-social-sdk" element so the array
items (e.g., "./apps/web/.next", "node_modules", "*.lock",
"apps/web/stellar-social-sdk", "**/node_modules/**") are properly separated and
the file parses as valid JSON.
♻️ Duplicate comments (1)
apps/backend/src/services/sync.service.ts (1)
908-955: Guard against null RPC data and avoid success+error return.Two issues remain:
datacan be null/undefined beforedata.success, and returning{ success: true, error: 'DUPLICATE_EVENT' }is ambiguous. Consider a null guard and a clearerskippedflag.🔧 Suggested fix
- ): Promise<{ success: boolean; syncEventId?: string; error?: string }> { + ): Promise<{ success: boolean; syncEventId?: string; skipped?: boolean; error?: string }> { const log = await loggingService.logBlockchainOperation('processEventAtomic', { eventId, eventType, bookingId, newStatus, }); try { const { data, error } = await supabase.rpc('process_sync_event_atomic', { p_event_id: eventId, p_event_type: eventType, p_booking_id: bookingId, p_property_id: propertyId, p_user_id: userId, p_event_data: eventData, p_new_status: newStatus || null, }); if (error) { await loggingService.logBlockchainError(log, { error, context: 'RPC call failed' }); throw new SyncError('Failed to process event atomically', 'ATOMIC_PROCESS_FAIL', error); } + if (!data) { + await loggingService.logBlockchainError(log, { + error: 'RPC returned no data', + context: 'processEventAtomic', + }); + throw new SyncError('RPC returned no data', 'ATOMIC_PROCESS_FAIL', { eventId }); + } + if (!data.success) { if (data.error === 'DUPLICATE_EVENT') { // Duplicate events are not errors, just skip them await loggingService.logBlockchainSuccess(log, { skipped: true, reason: 'duplicate' }); - return { success: true, error: 'DUPLICATE_EVENT' }; + return { success: true, skipped: true }; } throw new SyncError(`Atomic processing failed: ${data.error}`, data.error, data); }
🧹 Nitpick comments (1)
biome.json (1)
11-29: Consider consolidating duplicate ignore patterns for clarity.There are redundant entries (
node_modules,coverage,*.lock,apps/backend/src/config/supabase.ts, etc.) alongside broader globs. Consider keeping only one form to reduce noise and avoid confusion.♻️ Example consolidation (one possible approach)
- "node_modules", - "*.lock", - "coverage", - "apps/backend/src/config/supabase.ts", + "**/node_modules/**", + "**/*.lock", + "**/coverage/**",
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/app/login/page.tsx`:
- Around line 140-150: The footer links to "/terms" and "/privacy" but those
routes are missing; create two simple React page components (e.g., TermsPage and
PrivacyPage) that export default components rendering the respective legal
content and wire them to the application routing so visiting /terms and /privacy
no longer 404; ensure each page returns valid JSX/TSX with a heading and
explanatory text and matches the app's layout conventions (same
styling/container used by other pages) and export default for Next.js/React
routing to pick up.
♻️ Duplicate comments (2)
apps/web/stellar-social-sdk/package.json (1)
17-20: Revisitcrypto-jsfor wallet‑grade cryptography.If
crypto-jsis used for key/seed derivation or other wallet‑critical operations, prefer platform crypto (crypto.subtle) and remove the dependency. This aligns with the earlier security concern already raised for this package. Line 18–20.#!/bin/bash # Locate crypto-js usage to confirm what it's used for. rg -n --type ts --type js -C 2 "crypto-js|CryptoJS"apps/web/src/hooks/auth/use-auth.tsx (1)
123-156: Account restoration for Google users leaves them in a partial state.For Freighter users, the account is restored via silent reconnect. For Google users, the comment states they must re-authenticate to get the account (lines 148-149), but there's no UI indication or mechanism to prompt this. Users see
isAuthenticated: true(sinceuseris set) butaccountremainsnull, which will causegetBalance()andsendPayment()to fail silently or throw.Consider either:
- Gating
isAuthenticatedonaccount(as suggested in past review)- Adding a flag like
needsReauthto prompt the UI to show re-authentication🐛 Option 1: Gate isAuthenticated on account
- const isAuthenticated = !!user && !!user.publicKey; + const isAuthenticated = !!user && !!user.publicKey && !!account;This ensures the app doesn't consider the user "authenticated" until their account is fully available.
🧹 Nitpick comments (7)
apps/web/src/components/auth/FreighterFallback.tsx (1)
39-59: Consider resetting error state when retrying connection.When a user clicks "Connect with Freighter" after a previous error, the error message persists during the connecting state because
setErrorMessage(null)is only called after theisFreighterInstalledcheck passes. If the user somehow triggershandleConnectwhen Freighter is not installed, the old error message would be replaced, but the state transitions could be cleaner.Additionally, consider adding early return if already connecting to prevent duplicate connection attempts.
♻️ Suggested improvement
const handleConnect = async () => { + if (isConnecting) return; + + setErrorMessage(null); + if (!isFreighterInstalled) { setState('error'); setErrorMessage('Freighter is not installed'); return; } setState('connecting'); - setErrorMessage(null); try {apps/web/src/hooks/auth/use-auth.tsx (4)
158-188: Google OAuth setup has potential issues.
Cleanup missing: The effect doesn't return a cleanup function. If the component remounts,
window.handleGoogleCredentialand Google initialization could be set up multiple times.Recursive setTimeout without limit: The retry mechanism (line 182) could loop indefinitely if Google script never loads. Consider adding a max retry count.
Magic numbers: The 500ms and 1000ms timeouts are undocumented and may not work reliably across network conditions.
♻️ Suggested improvement with cleanup and retry limit
useEffect(() => { if (!sdk || !GOOGLE_CLIENT_ID) return; + let retryCount = 0; + const MAX_RETRIES = 10; + let timeoutId: ReturnType<typeof setTimeout> | null = null; + const setupGoogleOAuth = () => { if (typeof window !== 'undefined' && window.google?.accounts?.id) { // Asignar callback global window.handleGoogleCredential = handleGoogleAuthComplete; // Inicializar Google Identity Services window.google.accounts.id.initialize({ client_id: GOOGLE_CLIENT_ID, callback: handleGoogleAuthComplete, auto_select: false, cancel_on_tap_outside: false, ux_mode: 'popup', context: 'signin', itp_support: true, use_fedcm_for_prompt: true, }); console.log('✅ Google OAuth initialized'); } else { - // Reintentar si el script de Google aún no ha cargado - setTimeout(setupGoogleOAuth, 500); + retryCount++; + if (retryCount < MAX_RETRIES) { + timeoutId = setTimeout(setupGoogleOAuth, 500); + } else { + console.warn('Google Identity Services failed to load after max retries'); + } } }; // Esperar a que cargue el script de Google - setTimeout(setupGoogleOAuth, 1000); + timeoutId = setTimeout(setupGoogleOAuth, 1000); + + return () => { + if (timeoutId) clearTimeout(timeoutId); + }; }, [sdk]);
210-220: Potential null pointer access onauthMethodData.If
stellarAccount.data.authMethodsis empty,authMethodDatawill beundefined, and accessingauthMethodData?.metadata?.nameis safe, but the pattern suggests an assumption that there's always at least one auth method.Consider adding a defensive check or logging a warning if
authMethodsis empty.♻️ Suggested defensive check
if (result.success && result.account) { const stellarAccount = result.account as StellarSocialAccount; const authMethodData = stellarAccount.data.authMethods[0]; + + if (!authMethodData) { + console.warn('No auth methods found in account data'); + } const socialUser: SocialUser = {
298-315: Logout checksauthMethodafter it's been cleared.On line 303,
setAuthMethod(null)is called, but on line 309, the code checksauthMethod === 'google'. Due to React's state batching,authMethodstill holds the old value at line 309, so this works, but it's confusing and fragile. Consider capturing the value before clearing.♻️ Clearer approach
const logout = useCallback(() => { + const wasGoogleAuth = authMethod === 'google'; + // Limpiar estado setUser(null); setAccount(null); setAuthMethod(null); // Limpiar localStorage clearStorage(); // Revocar acceso de Google si estaba autenticado con Google - if (authMethod === 'google' && window.google?.accounts?.id) { + if (wasGoogleAuth && window.google?.accounts?.id) { window.google.accounts.id.disableAutoSelect(); }
317-331:getBalancesilently returns empty array on missing account.While this avoids throwing, it may mask issues where the UI expects balance data but receives an empty array due to session state issues. Consider logging at a higher severity or returning a more explicit result type.
apps/web/src/app/login/page.tsx (1)
34-44: Inconsistent redirect behavior between Google and Freighter login.
FreighterFallbackreceivesonSuccess={handleSuccess}(line 105) which triggers an immediate redirect, whileSocialLoginButton(line 92) has noonSuccessprop. Google login relies on theuseEffectwatchingisAuthenticatedto trigger the redirect.This creates different UX paths:
- Freighter: Connect →
handleSuccess→router.push(immediate)- Google: Login → state updates → useEffect fires →
router.push(next render cycle)Both work, but for consistency and clearer data flow, consider passing
onSuccesstoSocialLoginButtonas well, or relying solely on theuseEffectfor both.apps/backend/src/middleware/error.middleware.ts (1)
60-80: DRY the domain error branches.The three branches are identical except the default status; a small helper keeps this from drifting.
♻️ Suggested refactor
@@ let statusCode = 500; const response: ErrorResponse = { error: 'Internal server error' }; + const applyDomainError = ( + domainErr: { code: string; message: string; details?: unknown }, + fallbackStatus: number + ) => { + statusCode = errorCodeToStatus[domainErr.code] ?? fallbackStatus; + response.error = domainErr.message; + response.code = domainErr.code; + if (domainErr.details && process.env.NODE_ENV !== 'production') { + response.details = domainErr.details; + } + }; @@ - } else if (err instanceof BookingError) { - statusCode = errorCodeToStatus[err.code] || 400; - response.error = err.message; - response.code = err.code; - if (err.details && process.env.NODE_ENV !== 'production') { - response.details = err.details; - } - } else if (err instanceof EscrowError) { - statusCode = errorCodeToStatus[err.code] || 502; - response.error = err.message; - response.code = err.code; - if (err.details && process.env.NODE_ENV !== 'production') { - response.details = err.details; - } - } else if (err instanceof SyncError) { - statusCode = errorCodeToStatus[err.code] || 500; - response.error = err.message; - response.code = err.code; - if (err.details && process.env.NODE_ENV !== 'production') { - response.details = err.details; - } - } else if (err.name === 'PostgrestError') { + } else if (err instanceof BookingError) { + applyDomainError(err, 400); + } else if (err instanceof EscrowError) { + applyDomainError(err, 502); + } else if (err instanceof SyncError) { + applyDomainError(err, 500); + } else if (err.name === 'PostgrestError') {
Hey @respp! All done 🙌
Also addressed the CodeRabbit suggestions that made sense (open redirect, Let me know if anything's missing! 🫡 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/backend/src/blockchain/eventListener.ts`:
- Around line 420-432: The getStatusForEventType in eventListener.ts is missing
mappings compared to sync.service.ts, causing some events to not yield a status;
update eventListener.getStatusForEventType to either call
syncService.getStatusForEventType(...) or extend its switch to include
'escrow_created' -> 'escrow_created', 'escrow_released' -> 'released' (or the
same strings used in sync.service.ts), and for
'booking_created'/'booking_updated' return the status from the incoming
eventData when present; ensure processEventAtomic continues to receive a
consistent status value for all event types.
🧹 Nitpick comments (6)
apps/backend/src/services/sync.service.ts (4)
658-717: Code duplication:isEventAlreadyProcessedandgetStatusForEventTypeare duplicated.These helper methods are implemented identically in both
sync.service.tsandeventListener.ts. SinceeventListener.tsalready importssyncService, consider exposing these as public methods onSyncServiceand reusing them inBlockchainEventListener.♻️ Suggested approach
- Make these methods public in
SyncService:- private async isEventAlreadyProcessed(eventId: string): Promise<boolean> { + async isEventAlreadyProcessed(eventId: string): Promise<boolean> {- private getStatusForEventType( + getStatusForEventType(
- In
eventListener.ts, remove the duplicate implementations and call:const isDuplicate = await syncService.isEventAlreadyProcessed(event.id); const newStatus = syncService.getStatusForEventType(event.type);Note: The
getStatusForEventTypeineventListener.tshas fewer cases (only handlesbooking_cancelledandpayment_confirmed), whilesync.service.tshandles more event types. Consolidating to the more complete version would be beneficial.
619-656: Consider simplifying the error logging fallback.The nested try-catch pattern is defensive, but converting
parseStellarEventDatato async solely for error logging adds unnecessary complexity. Consider using fire-and-forget logging (without awaiting) similar to the pattern ineventListener.ts.♻️ Suggested simplification
- private async parseStellarEventData(event: { + private parseStellarEventData(event: { value?: Record<string, unknown>; - }): Promise<BlockchainEventData> { + }): BlockchainEventData { try { // ... parsing logic unchanged ... } catch (error) { - try { - const errorLog = await loggingService.logBlockchainOperation('parseStellarEventData', { - event: event.value, - }); - await loggingService.logBlockchainError(errorLog, { - error, - context: 'Failed to parse event data - returning empty object', - }); - } catch (loggingError) { - console.error('Failed to log parseStellarEventData error:', loggingError); - console.error('Original parsing error:', error); - } + // Fire-and-forget logging + loggingService + .logBlockchainOperation('parseStellarEventData', { event: event.value }) + .then((errorLog) => loggingService.logBlockchainError(errorLog, { + error, + context: 'Failed to parse event data - returning empty object', + })) + .catch((loggingError) => { + console.error('Failed to log parseStellarEventData error:', loggingError); + console.error('Original parsing error:', error); + }); return {}; } }This keeps the method synchronous and avoids blocking the sync flow for logging.
916-934: Consider removing or deprecating unused handlers.
handleBookingCancelledandhandlePaymentConfirmedare now empty since status updates are handled atomically. They're not called fromprocessEvent. Consider either removing them or adding@deprecatedannotations if kept for reference.
1093-1121: Remove or deprecate unused methods after atomic refactor.
storeSyncEventandmarkEventProcessedare no longer called anywhere in the codebase—all event handling now goes throughprocessEventAtomic. Mark these methods with@deprecatedannotations or remove them. The eventListener.ts file provides a pattern for this (see the deprecatedstoreEventmethod).apps/backend/src/blockchain/eventListener.ts (2)
527-536: Consider throwing an error in deprecated method.The deprecation warning is good, but the method silently does nothing. If accidentally called, the event would be silently dropped. Consider throwing to make misuse obvious during development.
♻️ Suggested change
private async storeEvent(_event: BlockchainEvent): Promise<void> { - // This method is deprecated - events are now stored atomically via processEventAtomic - // Keeping for backward compatibility but it should not be called - console.warn('storeEvent is deprecated - use processEventAtomic instead'); + throw new Error('storeEvent is deprecated - use processEventAtomic instead'); }Or if you need to keep it callable for tests, at minimum add a return statement and mark with
@deprecatedJSDoc:/** * `@deprecated` Use processEventAtomic instead. This method is a no-op. */ private async storeEvent(_event: BlockchainEvent): Promise<void> { console.warn('storeEvent is deprecated - use processEventAtomic instead'); return; }
383-418: Code duplication withsync.service.ts.This method is nearly identical to
isEventAlreadyProcessedinsync.service.ts. Consider reusing viasyncService.isEventAlreadyProcessed()sincesyncServiceis already imported.
| /** | ||
| * Determine the new status for an event type | ||
| */ | ||
| private getStatusForEventType(eventType: string): string | undefined { | ||
| switch (eventType) { | ||
| case 'booking_cancelled': | ||
| return 'cancelled'; | ||
| case 'payment_confirmed': | ||
| return 'confirmed'; | ||
| default: | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete getStatusForEventType compared to sync.service.ts.
This version only maps booking_cancelled and payment_confirmed, while the sync.service.ts version also handles escrow_created, escrow_released, and passes through status from eventData for booking_created/booking_updated.
If processEventAtomic expects consistent status mapping, this could cause status updates to be missed for certain event types.
🔧 Suggested fix
Either consolidate to use syncService.getStatusForEventType() (as suggested in the other file), or align the implementations:
private getStatusForEventType(eventType: string): string | undefined {
switch (eventType) {
case 'booking_cancelled':
return 'cancelled';
case 'payment_confirmed':
return 'confirmed';
+ case 'booking_created':
+ case 'booking_updated':
+ // Status should come from event data, handled by processEventAtomic
+ return undefined;
default:
return undefined;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Determine the new status for an event type | |
| */ | |
| private getStatusForEventType(eventType: string): string | undefined { | |
| switch (eventType) { | |
| case 'booking_cancelled': | |
| return 'cancelled'; | |
| case 'payment_confirmed': | |
| return 'confirmed'; | |
| default: | |
| return undefined; | |
| } | |
| } | |
| /** | |
| * Determine the new status for an event type | |
| */ | |
| private getStatusForEventType(eventType: string): string | undefined { | |
| switch (eventType) { | |
| case 'booking_cancelled': | |
| return 'cancelled'; | |
| case 'payment_confirmed': | |
| return 'confirmed'; | |
| case 'booking_created': | |
| case 'booking_updated': | |
| // Status should come from event data, handled by processEventAtomic | |
| return undefined; | |
| default: | |
| return undefined; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/backend/src/blockchain/eventListener.ts` around lines 420 - 432, The
getStatusForEventType in eventListener.ts is missing mappings compared to
sync.service.ts, causing some events to not yield a status; update
eventListener.getStatusForEventType to either call
syncService.getStatusForEventType(...) or extend its switch to include
'escrow_created' -> 'escrow_created', 'escrow_released' -> 'released' (or the
same strings used in sync.service.ts), and for
'booking_created'/'booking_updated' return the status from the incoming
eventData when present; ensure processEventAtomic continues to receive a
consistent status value for all event types.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/backend/src/services/sync.service.ts`:
- Around line 639-662: The duplicate-check currently treats any existing
sync_events row as a duplicate; in isEventAlreadyProcessed change the logic to
only consider an event a duplicate when its processed flag is true — either by
modifying the query to .eq('event_id', eventId).eq('processed', true) or by
keeping the existing select and returning Boolean(data && data.processed); keep
the existing error logging (loggingService.logBlockchainOperation /
logBlockchainError) behavior intact and ensure the function returns false for
rows where processed === false so previously-failed events can be retried.
| * Check if event has already been processed (duplicate check) | ||
| */ | ||
| private async isEventAlreadyProcessed(eventId: string): Promise<boolean> { | ||
| try { | ||
| const { data, error } = await supabase | ||
| .from('sync_events') | ||
| .select('id, processed') | ||
| .eq('event_id', eventId) | ||
| .single(); | ||
|
|
||
| if (error && error.code !== 'PGRST116') { | ||
| // PGRST116 is "not found" which is expected for new events | ||
| // Other errors should be logged but not block processing | ||
| const errorLog = await loggingService.logBlockchainOperation('isEventAlreadyProcessed', { | ||
| eventId, | ||
| }); | ||
| await loggingService.logBlockchainError(errorLog, { | ||
| error, | ||
| context: 'Error checking for duplicate event', | ||
| }); | ||
| } | ||
|
|
||
| return data !== null && data !== undefined; | ||
| } catch (error) { |
There was a problem hiding this comment.
Duplicate check should honor processed to avoid skipping unprocessed events.
Right now any existing row (even processed=false) causes a skip, which can permanently drop a previously failed event. Consider only treating events as duplicates when processed is true.
🔧 Suggested fix
- return data !== null && data !== undefined;
+ return data?.processed === true;🤖 Prompt for AI Agents
In `@apps/backend/src/services/sync.service.ts` around lines 639 - 662, The
duplicate-check currently treats any existing sync_events row as a duplicate; in
isEventAlreadyProcessed change the logic to only consider an event a duplicate
when its processed flag is true — either by modifying the query to
.eq('event_id', eventId).eq('processed', true) or by keeping the existing select
and returning Boolean(data && data.processed); keep the existing error logging
(loggingService.logBlockchainOperation / logBlockchainError) behavior intact and
ensure the function returns false for rows where processed === false so
previously-failed events can be retried.
|
Merged manually into main via commit df05d3b. Thanks @AnoukRImola for the contribution! |
2 participants
Pull Request | StellarRent
📝 Summary
Replace the existing email/password authentication system with social login using the Stellar-Account-Abstraction-SDK. Users can now sign in with Google and automatically receive a Stellar wallet through account abstraction, eliminating the need to manage private keys.
🔗 Related Issues
Closes: #199
🔄 Changes Made
Frontend (apps/web):
SocialLoginButtoncomponent for Google Sign-InFreighterFallbackcomponent for manual wallet connectionuseAuthhook with session persistence (localStorage)SDK Integration:
@stellar/freighter-api(v4.1.0)Backend:
_deprecated/)Config:
🖼️ Current Output
🧪 Testing
✅ Testing Checklist
use-auth.test.tsx- 305 lines covering login/logout flows)Manual Testing Performed:
🚀 Next Steps & Improvements
💬 Comments
The account abstraction approach generates deterministic wallets from Google credentials - same user always gets the same wallet address. This greatly simplifies onboarding for non-crypto users.
Summary by CodeRabbit
New Features
Backend Improvements
✏️ Tip: You can customize this high-level summary in your review settings.