Tg identities v2 implementation #468
Conversation
Implements full telegram identity system integration including: - Add telegram incentive methods to IncentiveManager (telegramLinked/telegramUnlinked) - Add getAccountByTelegramUsername RPC endpoint for external system queries - Implement GCR.getAccountByTelegramUsername() with database JSONB queries - Add complete PointSystem telegram support: - LINK_TELEGRAM: 2 points value - awardTelegramPoints() with ownership verification - deductTelegramPoints() with ownership verification - Updated socialAccounts structure to include telegram - Add telegram support to GCR_Main entity type definitions The telegram bot can now submit identity transactions that are fully processed by the node's incentive and storage systems. Users receive 2 points for linking telegram accounts with proper anti-abuse measures. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
… it is done by bots
WalkthroughAdds Telegram web2 identity (dual‑signature verification and genesis-based bot authorization), PointSystem/incentive Telegram support, genesis identities loader and SharedState field, GitHub Actions to preserve/notify Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant DApp
participant Bot as Telegram Bot
participant Node
participant Genesis
participant GCR
User->>DApp: request link (wallet-signed challenge)
DApp->>Bot: forward challenge
Bot->>Bot: verify user wallet signature
Bot->>Bot: create TelegramSignedAttestation (bot-signed)
Bot->>Node: submit identity tx (identity / subtype: web2_platform {platform: "telegram", payload})
Node->>Genesis: check bot authorization (genesisIdentities + min balance)
Node->>Node: verify bot signature and payload integrity
alt verification ok
Node->>GCR: add Telegram identity
Node->>IncentiveManager: telegramLinked (if first connection)
Node-->>Bot: success
else verification failed
Node-->>Bot: reject (error)
end
sequenceDiagram
autonumber
participant GH as GitHubActions
participant Repo
Note over GH,Repo: On push (merge commit detection)
GH->>Repo: inspect latest commit parents and changed files
alt commit is merge AND .claude/ or .serena/ changed
GH->>Repo: checkout MERGE_BASE -- .claude/ (or .serena/)
GH->>Repo: git add & commit "[skip ci] restore .claude/ or .serena/"
GH->>Repo: push revert to current branch
else
GH-->>Repo: exit (no-op)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/features/incentive/PointSystem.ts (2)
112-120: Normalize socialAccounts shape to always include telegram/github/twitter/discordIf socialAccounts exists but lacks telegram, callers may read undefined. Merge defaults to guarantee keys exist.
breakdown: { web3Wallets: account.points.breakdown?.web3Wallets || {}, - socialAccounts: account.points.breakdown?.socialAccounts || { - twitter: 0, - github: 0, - telegram: 0, - discord: 0, - }, + socialAccounts: { + twitter: 0, + github: 0, + telegram: 0, + discord: 0, + ...(account.points.breakdown?.socialAccounts || {}), + }, referrals: account.points.breakdown?.referrals || 0, demosFollow: account.points.breakdown?.demosFollow || 0,
195-206: Potential NPE: account.points.breakdown.socialAccounts may be undefinedExisting accounts might not have initialized breakdown/socialAccounts. Initialize before mutate.
if ( type === "socialAccounts" && (platform === "twitter" || platform === "github" || platform === "telegram" || platform === "discord") ) { + // Ensure structure exists + account.points.breakdown = + account.points.breakdown || { + web3Wallets: {}, + socialAccounts: { twitter: 0, github: 0, telegram: 0, discord: 0 }, + referrals: 0, + demosFollow: 0, + } + account.points.breakdown.socialAccounts = + account.points.breakdown.socialAccounts || { + twitter: 0, + github: 0, + telegram: 0, + discord: 0, + } const oldPlatformPoints = account.points.breakdown?.socialAccounts?.[platform] || 0 account.points.breakdown.socialAccounts[platform] = oldPlatformPoints + points
🧹 Nitpick comments (26)
src/utilities/validateUint8Array.ts (1)
3-6: Sorting by Number(a)-Number(b) can yield NaN; filter numeric keys explicitlyFor non‑numeric keys,
Number(key)isNaN, making comparisons ill‑defined. Filtering to/^\d+$/and then sorting numerically is safer and avoids surprises. Also, for arrays/typed arrays, sorting is unnecessary; their iteration order is already index order..gitignore (1)
129-139: Deduplicate Serena cache ignoreNow that .serena/.gitignore ignores /cache, the explicit .serena/cache/... pattern here is redundant.
Apply this diff:
- .serena/cache/typescript/document_symbols_cache_v23-06-25.pkl.github/workflows/claude-merge-fix.yml (2)
52-56: Simplify staging logicgit checkout -- .claude/ already updates the index; the staged‑diff guard + add is redundant and inverted. Safe to drop it.
Apply this diff:
- if git diff --staged --quiet; then - git add .claude/ - fi + # index is updated by checkout; no extra add needed
46-47: Name correctness: this is first parent, not merge‑baseVariable MERGE_BASE actually holds the first parent of the merge; consider renaming to PARENT1 for clarity.
Apply this diff:
- MERGE_BASE=$(git log -1 --pretty=format:"%P" | cut -d' ' -f1) - git checkout $MERGE_BASE -- .claude/ 2>/dev/null || echo "No .claude in base commit" + PARENT1=$(git log -1 --pretty=format:"%P" | cut -d' ' -f1) + git checkout "$PARENT1" -- .claude/ 2>/dev/null || echo "No .claude in base commit".github/workflows/notify-serena-merging.yml (1)
27-33: Harden gh usageIf gh isn’t available on some runners, install/setup or fall back to log‑only. Optional but improves reliability.
Apply this diff to install gh if missing:
- gh pr comment ${{ github.event.number }} --body "⚠️ **MCP Memory Files Detected** + command -v gh >/dev/null 2>&1 || sudo apt-get update && sudo apt-get install -y gh || true + gh pr comment ${{ github.event.number }} --body "⚠️ **MCP Memory Files Detected**src/libs/abstraction/index.ts (3)
86-95: Normalize Telegram username casing and ID typesTelegram usernames are case-insensitive; IDs may be string/number. Current strict equality risks false negatives.
Apply this diff:
- if ( - telegramAttestation.payload.telegram_id !== payload.userId || - telegramAttestation.payload.username !== payload.username - ) { + const idMatches = + String(telegramAttestation.payload.telegram_id) === String(payload.userId) + const usernameMatches = + telegramAttestation.payload.username?.toLowerCase() === payload.username?.toLowerCase() + if (!idMatches || !usernameMatches) { return { success: false, message: "Telegram attestation data mismatch", } }
131-138: Authorization model noteGenesis-balance presence is a one-way allow; it can’t handle bot revocation/rotation without a new genesis. Consider augmenting with an on-chain allowlist/param set or a signed bot-registry state.
204-208: Guard string coercion for non-telegram contextsIf payload.proof is unexpectedly non-string, readData will throw. Add a fast type check to fail early with a clearer message.
Apply this diff:
- const { message, type, signature } = await instance.readData( - payload.proof as string, - ) + if (typeof payload.proof !== "string") { + return { success: false, message: "Invalid proof format (expected string)" } + } + const { message, type, signature } = await instance.readData(payload.proof).serena/project.yml (2)
10-16: Consider adding repo-specific ignores instead of relying solely on .gitignoreIf large artifacts (e.g., logs, temp data) aren’t in .gitignore, add them here to prevent tool churn.
62-62: Keep excluded_tools empty (good), but document rationaleA brief inline comment stating “intentionally empty to keep full tool surface” helps future maintainers.
.serena/memories/project_context_consolidated.md (3)
11-22: Fix fenced code block language for the directory treeAdd a language to satisfy markdownlint MD040 and improve rendering. For trees, use “text”.
Apply this diff:
-``` +```text src/ ├── features/ # Feature modules (multichain, incentives) ... -``` +```
25-37: Annotate shell blocks with bashLabel code fences to satisfy MD040 and enable syntax highlighting.
Apply this diff:
-```bash +```bash # Code Quality (REQUIRED after changes) bun run lint:fix # ESLint validation + auto-fix ... -``` +```
8-8: Avoid committing machine-specific absolute pathsReplace personal absolute paths with placeholders (e.g., “/node”) to keep docs portable.
.github/workflows/claude-merge-notify.yml (1)
21-33: Use explicit SHAs for diff to avoid missing refs; safer heredoc for comment bodyorigin/${{ github.base_ref }} may not exist locally; HEREDOC avoids quoting pitfalls.
Apply this diff:
- if git diff --name-only origin/${{ github.base_ref }}...HEAD | grep -q "^\.claude/"; then + BASE_SHA="${{ github.event.pull_request.base.sha }}" + HEAD_SHA="${{ github.event.pull_request.head.sha }}" + if git diff --name-only "$BASE_SHA...$HEAD_SHA" | grep -q "^\.claude/"; then echo "⚠️ This PR modifies .claude/ files" - - COMMENT_BODY="⚠️ **Claude Memory Files Detected** - - This PR modifies \`.claude/\` files. After merge, these changes will be **automatically reverted** to preserve branch-specific Claude conversation context. - - **Files that will be reverted:** - $(git diff --name-only origin/${{ github.base_ref }}...HEAD | grep '^\.claude/' | sed 's/^/- /' | head -10) - - This is expected behavior to keep Claude conversation context branch-specific. ✅" - - gh pr comment ${{ github.event.number }} --body "$COMMENT_BODY" || echo "Could not post comment" + FILE_LIST="$(git diff --name-only "$BASE_SHA...$HEAD_SHA" | grep '^\.claude/' | sed 's/^/- /' | head -10)" + cat > body.md <<'EOF' +⚠️ **Claude Memory Files Detected** + +This PR modifies `.claude/` files. After merge, these changes will be **automatically reverted** to preserve branch-specific Claude conversation context. + +**Files that will be reverted:** +EOF + echo "$FILE_LIST" >> body.md + echo "" >> body.md + echo "This is expected behavior to keep Claude conversation context branch-specific. ✅" >> body.md + gh pr comment ${{ github.event.number }} --body-file body.md || echo "Could not post comment".github/workflows/fix-serena-conflicts.yml (2)
28-39: Detect changes in merge commits correctlygit log -1 --name-only often yields nothing for merge commits without -m. Use git show -m.
Apply this diff:
- if git log -1 --name-only | grep -q "^\.serena/"; then + if git show -m --name-only --pretty=format: HEAD | grep -q "^\.serena/"; then
47-50: Variable naming and base resolutionThis is the first parent, not a “merge base”. Rename for clarity. Also ensure the commit is present locally (safe with fetch-depth: 0).
Apply this diff:
- MERGE_BASE=$(git log -1 --pretty=format:"%P" | cut -d' ' -f1) + FIRST_PARENT=$(git log -1 --pretty=format:"%P" | cut -d' ' -f1) @@ - git checkout $MERGE_BASE -- .serena/ 2>/dev/null || echo "No .serena in base commit" + git checkout "$FIRST_PARENT" -- .serena/ 2>/dev/null || echo "No .serena in base commit"src/features/incentive/PointSystem.ts (1)
733-746: Guard duplicate-award check against missing keyUse nullish coalescing to avoid undefined comparisons and ensure idempotency.
- // Check if user already has Telegram points specifically - if (userPointsWithIdentities.breakdown.socialAccounts.telegram > 0) { + // Check if user already has Telegram points specifically + const currentTelegram = + userPointsWithIdentities.breakdown.socialAccounts?.telegram ?? 0 + if (currentTelegram > 0) { return { result: 200, response: { pointsAwarded: 0, totalPoints: userPointsWithIdentities.totalPoints, message: "Telegram points already awarded", },.serena/memories/telegram_identity_system_complete.md (2)
24-29: Add fenced code language for the step blockSilences MD040 and improves rendering.
-``` +```text 1. User signs payload in Telegram bot (bot verifies locally) 2. Bot creates TelegramSignedAttestation with bot signature 3. Node verifies bot signature + bot authorization 4. User ownership validated via public key matching--- `13-15`: **Align terminology: it’s bot‑signature validation + sender PK match, not “dual signature”** Doc elsewhere states the attestation signature is from the bot. Rephrase Phase 4a to avoid confusion. ```diff -- **Phase 4a** ✅: Cryptographic Dual Signature Validation +- **Phase 4a** ✅: Bot Signature Validation + Sender Public‑Key MatchTG_IDENTITY_PLAN.md (4)
10-19: Update flow and headings: remove trailing colon and reflect bot‑signature modelPlan still mentions “dual signatures (user + bot)”. Implementation uses bot‑signed attestation with sender PK match. Also fix MD026.
-### Complete Transaction-Based Flow: +### Complete Transaction-Based Flow @@ -6. **BOT** creates attestation with dual signatures (user + bot) +6. **BOT** creates TelegramSignedAttestation (bot signature), referencing the user's signed challenge
99-107: Security section: align wording with implemented verificationReplace “Dual Signature Verification” with precise checks; remove trailing colon in headings flagged by MD026.
-## Security Features +## Security Features @@ -2. **Dual Signature Verification**: User signature + Bot signature required -3. **Bot Authorization**: Only genesis addresses can create valid bot signatures +2. **Bot Signature Verification**: Node verifies bot signature on attestation; user ownership enforced via sender public key match +3. **Bot Authorization**: Only genesis-authorized bot addresses are accepted
135-159: Add language to fenced diagram blockSilences MD040 and improves readability.
-``` +```text ┌─────────────────────────────────────────────────────────────────────────────┐ │ TRANSACTION-BASED FLOW │ └─────────────────────────────────────────────────────────────────────────────┘ @@ **Transaction Flow**: Bot creates transaction → Node validates through GCR → Identity stored **Security**: Dual signatures (user + bot) + Bot authorization via genesis addresses
110-120: Remove trailing colons in headings flagged by markdownlintMD026: “Completed (Bot):”, “To Do (../sdks repo):”, “To Do (Node repo):”.
-### ✅ **Completed (Bot)**: +### ✅ **Completed (Bot)** @@ -### ❌ **To Do (../sdks repo)**: +### ❌ **To Do (../sdks repo)** @@ -### ❌ **To Do (Node repo)**: +### ❌ **To Do (Node repo)**src/libs/network/server_rpc.ts (1)
336-344: Prefer jsonResponse() for consistent headersReturn JSON with correct Content‑Type and match the rest of the server.
- server.get("/", req => { - const clientIP = rateLimiter.getClientIP(req, server.server) - return new Response( - JSON.stringify({ - message: "Hello, World!", - yourIP: clientIP, - }), - ) - }) + server.get("/", req => { + const clientIP = rateLimiter.getClientIP(req, server.server) + return jsonResponse({ message: "Hello, World!", yourIP: clientIP }) + })src/libs/network/manageGCRRoutines.ts (1)
85-96: Normalize Telegram usernames (strip leading '@'); keep input validationPrevents lookup misses when clients include “@”.
- case "getAccountByTelegramUsername": { - const username = params[0] + case "getAccountByTelegramUsername": { + const usernameRaw = params[0] + const username = + typeof usernameRaw === "string" + ? usernameRaw.trim().replace(/^@/, "") + : ""If usernames are case-insensitive in storage, consider switching the DB query to ILIKE for robustness.
src/libs/blockchain/gcr/gcr.ts (1)
571-602: Telegram username lookup works; harden JSONB query and indexing
- Use COALESCE to avoid jsonb_array_elements errors when
identities.web2.telegramis missing.- Consider lower-casing usernames during write and lookup.
Apply COALESCE in the predicate:
- .where( - "EXISTS (SELECT 1 FROM jsonb_array_elements(gcr.identities->'web2'->'telegram') as telegram_id WHERE telegram_id->>'username' = :username)", - { username }, - ) + .where( + "EXISTS (SELECT 1 FROM jsonb_array_elements(COALESCE(gcr.identities->'web2'->'telegram','[]'::jsonb)) AS telegram_id WHERE telegram_id->>'username' = :username)", + { username }, + )Add a GIN index to keep these JSONB lookups fast:
CREATE INDEX IF NOT EXISTS gcrmain_identities_gin ON "GCR_Main" USING GIN ((identities) jsonb_path_ops);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (22)
.eslintignore(1 hunks).eslintrc.cjs(1 hunks).github/workflows/claude-merge-fix.yml(1 hunks).github/workflows/claude-merge-notify.yml(1 hunks).github/workflows/fix-serena-conflicts.yml(1 hunks).github/workflows/notify-serena-merging.yml(1 hunks).gitignore(1 hunks).serena/.gitignore(1 hunks).serena/memories/project_context_consolidated.md(1 hunks).serena/memories/telegram_identity_system_complete.md(1 hunks).serena/project.yml(1 hunks)TG_IDENTITY_PLAN.md(1 hunks)package.json(1 hunks)src/features/incentive/PointSystem.ts(4 hunks)src/libs/abstraction/index.ts(3 hunks)src/libs/blockchain/gcr/gcr.ts(1 hunks)src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts(14 hunks)src/libs/blockchain/gcr/gcr_routines/IncentiveManager.ts(1 hunks)src/libs/network/manageGCRRoutines.ts(1 hunks)src/libs/network/server_rpc.ts(1 hunks)src/model/entities/GCRv2/GCR_Main.ts(1 hunks)src/utilities/validateUint8Array.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/libs/network/manageGCRRoutines.ts (1)
src/libs/blockchain/gcr/gcr.ts (1)
GCR(109-979)
src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts (4)
src/libs/abstraction/index.ts (1)
verifyWeb2Proof(158-238)src/utilities/logger.ts (1)
error(125-132)src/libs/crypto/hashing.ts (1)
Hashing(15-26)src/libs/blockchain/gcr/gcr_routines/IncentiveManager.ts (1)
IncentiveManager(9-115)
src/features/incentive/PointSystem.ts (1)
src/libs/blockchain/gcr/gcr_routines/ensureGCRForUser.ts (1)
ensureGCRForUser(8-33)
🪛 markdownlint-cli2 (0.17.2)
.serena/memories/telegram_identity_system_complete.md
24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.serena/memories/project_context_consolidated.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
TG_IDENTITY_PLAN.md
10-10: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
110-110: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
116-116: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
120-120: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
129-129: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
135-135: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (15)
src/utilities/validateUint8Array.ts (1)
2-8: Avoid Node-only Buffer; return a true Uint8Array for cross‑env safetyReplace Buffer.from(...) with Uint8Array and fast‑path existing Uint8Array — suggested patch below.
Apply:
-export default function validateIfUint8Array(input: unknown): Uint8Array | unknown { - if (typeof input === "object" && input !== null) { - const txArray = Object.keys(input) - .sort((a, b) => Number(a) - Number(b)) - .map(k => input[k]) - return Buffer.from(txArray) - } - return input -} +export default function validateIfUint8Array<T>(input: T): Uint8Array | T { + // Fast path: already a typed array (Buffer also passes as it extends Uint8Array) + if (input instanceof Uint8Array) return input; + + // Array of numbers -> Uint8Array + if (Array.isArray(input)) { + return Uint8Array.from(input as number[], (v) => (v as number) & 0xff); + } + + // Object with numeric keys -> Uint8Array + if (input !== null && typeof input === "object") { + const obj = input as Record<string, unknown>; + const keys = Object.keys(obj) + .filter((k) => /^\d+$/.test(k)) + .map((k) => Number(k)) + .sort((a, b) => a - b); + const bytes = keys.map((k) => Number((obj as any)[k]) & 0xff); + return Uint8Array.from(bytes); + } + + // Anything else: pass through unchanged + return input; +}ripgrep run returned no matches for 'validateIfUint8Array' — verify imports/usages (or re-run a broader search) to confirm browser impact.
.eslintignore (1)
8-9: LGTM: targeted ESLint ignoresAdding postgres_* and aptos_examples_ts to ESLint ignores is reasonable and scoped.
.serena/.gitignore (1)
1-1: LGTM: ignore Serena cacheKeeps noise out of diffs and workflows.
.eslintrc.cjs (1)
41-41: LGTM: allow UPPER_CASE for variable‑like identifiersMatches new constant naming (e.g., LINK_TELEGRAM).
.gitignore (1)
129-130: Confirm dist ignore is intendeddist is now ignored; since package.json is private this is fine, but ensure any CI artifacts don’t rely on checking in dist.
src/libs/abstraction/index.ts (2)
171-173: LGTM: early exit for telegram pathRouting Telegram to its dedicated verifier avoids parser misuse and simplifies control flow.
117-123: Require explicit bot_public_key for signature verification (don’t use bot_address as the public key)You pass botAddress into ucrypto.verify; if bot_address is an address (not the raw public key) verification will fail. Require attestation.bot_public_key, use it for signature verification, then derive-and-cross-check the address before calling checkBotAuthorization. Location: src/libs/abstraction/index.ts (around lines 117–123).
Apply this diff (assuming attestation includes bot_public_key):
- const { public_key: userPublicKey, bot_address: botAddress } = + const { public_key: userPublicKey, bot_address: botAddress, bot_public_key: botPublicKey } = attestationPayload @@ - const botSignatureValid = await ucrypto.verify({ + const botSignatureValid = await ucrypto.verify({ algorithm: signature.type, message: new TextEncoder().encode(messageToVerify), - publicKey: hexToUint8Array(botAddress), // Bot's public key + publicKey: hexToUint8Array(botPublicKey), signature: hexToUint8Array(signature.data), }) @@ - const botAuthorized = await checkBotAuthorization(botAddress) + // Optional: derive address from public key and cross-check + // const derived = deriveAddressFromPublicKey(botPublicKey) + // if (derived.toLowerCase() !== botAddress.toLowerCase()) { ... } + const botAuthorized = await checkBotAuthorization(botAddress)Repo search (run earlier) found no existing address-derivation helper — implement deriveAddressFromPublicKey or add/rename the attestation field to avoid the footgun.
.serena/project.yml (1)
6-6: LGTM: explicit TypeScript project declarationMatches repo language and aligns with alias usage noted elsewhere.
src/features/incentive/PointSystem.ts (2)
16-16: Add LINK_TELEGRAM constant — LGTMValue aligns with Twitter. No concerns.
703-778: Parity: normalize social point checks & extract shared getSocialPointsTwitter/GitHub award/deduct paths use raw >0 / <=0 checks and can hit undefined/NaN edge cases; implement getSocialPoints(account, platform): number (fallback 0) and use it across all social award/deduct logic to DRY and harden behavior.
- Replace raw comparisons (e.g., breakdown.socialAccounts.twitter > 0) with the helper.
- Add unit tests for missing-key / undefined social breakdown scenarios.
Applies also to: src/features/incentive/PointSystem.ts lines 786–858
Would you like a follow‑up PR to refactor these paths and add tests?
.serena/memories/telegram_identity_system_complete.md (1)
80-86: Consistency check: “Double Protection” wordingIf only the bot signature is verified (plus sender PK match), avoid implying two cryptographic signatures. Consider “Bot signature + genesis authorization”.
Do we still verify a user signature on‑chain, or only inside the bot? If only inside the bot, I can adjust phrasing across docs for consistency.
src/libs/blockchain/gcr/gcr.ts (1)
563-569: Twitter tie-breaker logic remains consistentOptional chaining with a points-based pick is fine; trailing comma in call arg is harmless.
src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts (2)
343-349: Unlink: store removed identity only for GitHub/Telegram — intentional?Twitter unlinking doesn’t capture
userId, hencetwitterUnlinkedcan’t distinguish multiple Twitter connections historically. Confirm this matches the points deduction contract.
363-381: Unlink deductions — idempotency present; verify clamp to 0
- PointSystem.deductGithubPoints and deductTelegramPoints short‑circuit when userPointsWithIdentities.breakdown.socialAccounts.github/telegram <= 0 (PointSystem.ts — deductGithubPoints ~line 622 with github check ~648–651; deductTelegramPoints ~line 786 with telegram check ~813–815), so repeated calls are no‑ops.
- Verify the persistence/update path these methods call when applying the subtraction actually clamps stored totals at 0 (e.g., uses Math.max or similar) and that the write is atomic to avoid negative balances.
src/libs/blockchain/gcr/gcr_routines/IncentiveManager.ts (1)
87-107: Telegram hooks — PointSystem signatures match.
awardTelegramPoints(userId: string, telegramUserId: string, referralCode?: string) and deductTelegramPoints(userId: string, telegramUserId: string) are implemented in src/features/incentive/PointSystem.ts and their parameter order matches the IncentiveManager calls. No changes required.
| jobs: | ||
| preserve-claude: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Grant write permissions for push
The job pushes a commit; without explicit permissions, GITHUB_TOKEN may be read‑only and the push will fail.
Apply this diff near the top:
jobs:
preserve-claude:
runs-on: ubuntu-latest
+ permissions:
+ contents: write📝 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.
| jobs: | |
| preserve-claude: | |
| runs-on: ubuntu-latest | |
| jobs: | |
| preserve-claude: | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: write |
🤖 Prompt for AI Agents
.github/workflows/claude-merge-fix.yml lines 7-9: the workflow runs a job that
pushes a commit but doesn’t declare write permissions for GITHUB_TOKEN; add a
permissions block near the top of the file (before jobs) that grants contents:
write so the job can push (e.g., permissions: contents: write), ensuring the
push will succeed.
| name: Claude PR Warning | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: ['**'] | ||
| types: [opened, synchronize] | ||
|
|
||
| jobs: | ||
| claude-warning: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Grant minimal permissions for PR comments
Explicit permissions avoid surprises on hardened orgs.
Apply this diff:
name: Claude PR Warning
+
+permissions:
+ contents: read
+ pull-requests: write🤖 Prompt for AI Agents
.github/workflows/claude-merge-notify.yml lines 1-9: this workflow needs
explicit, minimal permissions to post PR comments on hardened orgs — add a
permissions block granting only the specific write scope used by your commenting
code (e.g., if you call the Pull Requests API grant pull-requests: write; if you
use the Issues API grant issues: write), include the permissions stanza at the
top-level of the workflow YAML so the runner has only that permission and
nothing broader.
| name: Preserve Branch-Specific Serena Files | ||
|
|
||
| on: | ||
| push: | ||
| branches: ['**'] | ||
|
|
||
| jobs: | ||
| preserve-serena: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit permissions and concurrency to avoid push races
Required to allow committing and pushing from the workflow, and to serialize runs per branch.
Apply this diff:
name: Preserve Branch-Specific Serena Files
+
+permissions:
+ contents: write
+
+concurrency:
+ group: preserve-serena-${{ github.ref }}
+ cancel-in-progress: false📝 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.
| name: Preserve Branch-Specific Serena Files | |
| on: | |
| push: | |
| branches: ['**'] | |
| jobs: | |
| preserve-serena: | |
| runs-on: ubuntu-latest | |
| name: Preserve Branch-Specific Serena Files | |
| permissions: | |
| contents: write | |
| concurrency: | |
| group: preserve-serena-${{ github.ref }} | |
| cancel-in-progress: false | |
| on: | |
| push: | |
| branches: ['**'] | |
| jobs: | |
| preserve-serena: | |
| runs-on: ubuntu-latest |
🤖 Prompt for AI Agents
In .github/workflows/fix-serena-conflicts.yml lines 1-9, the workflow lacks
explicit permissions and concurrency settings; add a top-level permissions block
granting the workflow write access to repository contents (e.g., permissions:
contents: write) so the job can commit and push, and add a top-level concurrency
block to serialize runs per branch (e.g., concurrency.group set to the branch
ref like github.ref and cancel-in-progress set appropriately) so concurrent
pushes on the same branch are serialized.
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 2 | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
There was a problem hiding this comment.
Fetch depth may be insufficient for parent retrieval
fetch-depth: 2 can miss the first parent tree. Fetch full history or at least the parent commit explicitly.
Apply this diff:
- with:
- fetch-depth: 2
- token: ${{ secrets.GITHUB_TOKEN }}
+ with:
+ fetch-depth: 0
+ token: ${{ secrets.GITHUB_TOKEN }}📝 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.
| uses: actions/checkout@v4 | |
| with: | |
| fetch-depth: 2 | |
| token: ${{ secrets.GITHUB_TOKEN }} | |
| uses: actions/checkout@v4 | |
| with: | |
| fetch-depth: 0 | |
| token: ${{ secrets.GITHUB_TOKEN }} |
🤖 Prompt for AI Agents
In .github/workflows/fix-serena-conflicts.yml around lines 12 to 16, the
checkout step uses fetch-depth: 2 which can omit parent commit history; change
the checkout configuration to fetch the full history by setting fetch-depth: 0
(or explicitly fetch the required parent commit) so parent trees are available
for merge/conflict resolution and CI operations.
| # Commit the reversion | ||
| if git diff --staged --quiet; then | ||
| git add .serena/ | ||
| fi | ||
| if ! git diff --cached --quiet; then | ||
| git commit -m "🔒 Preserve branch-specific .serena files | ||
| Reverted .serena/ changes from merge to keep $CURRENT_BRANCH version intact. | ||
| [skip ci]" | ||
| git push origin $CURRENT_BRANCH | ||
| echo "✅ Successfully preserved $CURRENT_BRANCH .serena files" | ||
| else | ||
| echo "ℹ️ No changes to revert" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify staging and commit logic
Use a straightforward add + check for staged changes; current staged check is inverted and brittle.
Apply this diff:
- # Commit the reversion
- if git diff --staged --quiet; then
- git add .serena/
- fi
-
- if ! git diff --cached --quiet; then
+ git add .serena/ || true
+ if ! git diff --cached --quiet -- .serena/; then
git commit -m "🔒 Preserve branch-specific .serena files
-
Reverted .serena/ changes from merge to keep $CURRENT_BRANCH version intact.
[skip ci]"
git push origin $CURRENT_BRANCH
echo "✅ Successfully preserved $CURRENT_BRANCH .serena files"
else
echo "ℹ️ No changes to revert"
fi📝 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.
| # Commit the reversion | |
| if git diff --staged --quiet; then | |
| git add .serena/ | |
| fi | |
| if ! git diff --cached --quiet; then | |
| git commit -m "🔒 Preserve branch-specific .serena files | |
| Reverted .serena/ changes from merge to keep $CURRENT_BRANCH version intact. | |
| [skip ci]" | |
| git push origin $CURRENT_BRANCH | |
| echo "✅ Successfully preserved $CURRENT_BRANCH .serena files" | |
| else | |
| echo "ℹ️ No changes to revert" | |
| fi | |
| git add .serena/ || true | |
| if ! git diff --cached --quiet -- .serena/; then | |
| git commit -m "🔒 Preserve branch-specific .serena files | |
| Reverted .serena/ changes from merge to keep $CURRENT_BRANCH version intact. | |
| [skip ci]" | |
| git push origin $CURRENT_BRANCH | |
| echo "✅ Successfully preserved $CURRENT_BRANCH .serena files" | |
| else | |
| echo "ℹ️ No changes to revert" | |
| fi |
🤖 Prompt for AI Agents
In .github/workflows/fix-serena-conflicts.yml around lines 56 to 71 the
staging/commit logic is inverted and brittle; replace the two-step conditional
with a simple sequence: run git add .serena/ unconditionally, then check for
staged changes with if ! git diff --staged --quiet; then perform the git commit
and git push and success echo, else echo no changes; remove the previous
conditional that only ran git add when git diff --staged --quiet to ensure
.serena files are always staged before the staged-changes check.
|
This PR modifies Files that will be reverted:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libs/network/manageGCRRoutines.ts (1)
10-13: Tighten payload typing (replaceany[]with a discriminated union).This prevents runtime type guards per method and catches param mistakes at compile time.
Sketch:
type GCRRoutinePayload = | { method: "getAccountByTelegramUsername"; params: [string] } | { method: "getAccountByIdentity"; params: [unknown /* Identity type */] } | { method: "getIdentities"; params: [string] } | { method: "getWeb2Identities"; params: [string] } | { method: "getXmIdentities"; params: [string] } | { method: "getPoints"; params: [string] } | { method: "validateReferralCode"; params: [string] } | { method: "identity_assign_from_write"; params: [unknown /* write */] } | { method: "getReferralInfo"; params: [string] }
🧹 Nitpick comments (1)
src/libs/network/manageGCRRoutines.ts (1)
95-96: Return a safe projection to avoid leaking full account records.
GCR.getAccountByTelegramUsername()likely returns a fullGCRMainentity. Consider returning only whitelisted fields (e.g.,pubkey, minimalidentities.web2.telegram, points summary) to limit exposure.Example (if acceptable here):
const acct = await GCR.getAccountByTelegramUsername(normalized) response.response = acct ? _.pick(acct, ["pubkey", "identities.web2.telegram", "points.total"]) : null
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/libs/blockchain/gcr/gcr.ts(3 hunks)src/libs/network/manageGCRRoutines.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libs/blockchain/gcr/gcr.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/libs/network/manageGCRRoutines.ts (1)
src/libs/blockchain/gcr/gcr.ts (1)
GCR(109-1050)
🔇 Additional comments (1)
src/libs/network/manageGCRRoutines.ts (1)
86-97: Confirm 200+null vs 404 semantics and downstream expectations.
- Evidence: GCR.getAccountByTelegramUsername returns null on miss (src/libs/blockchain/gcr/gcr.ts:642); manageGCRRoutines forwards that and keeps response.result = 200 (src/libs/network/manageGCRRoutines.ts — case "getAccountByTelegramUsername").
- Repo scan: no in-repo callers of this RPC found; other code typically treats response.result === 200 as success.
- Action: Decide whether "not found" should be a 404 or remain 200+null. If 404 is desired, set response.result = response.response ? 200 : 404 before returning (or have GCR return a full RPCResponse). If 200+null is intentional, document this and confirm external clients check response.response for existence (not just status).
| case "getAccountByTelegramUsername": { | ||
| const username = params[0] | ||
|
|
||
| if (!username) { | ||
| response.result = 400 | ||
| response.response = "No username specified" | ||
| break | ||
| } | ||
|
|
||
| response.response = await GCR.getAccountByTelegramUsername(username) | ||
| break | ||
| } |
There was a problem hiding this comment.
Normalize and validate Telegram usernames; align error shape with existing handlers.
- Accept both "@foo" and "foo", trim, and lowercase to avoid case/format mismatches with DB lookups.
- Validate type and format up front; return 400 with
response=nullandextra.error(same pattern as Line 76-79).
Apply this diff:
case "getAccountByTelegramUsername": {
- const username = params[0]
-
- if (!username) {
- response.result = 400
- response.response = "No username specified"
- break
- }
-
- response.response = await GCR.getAccountByTelegramUsername(username)
+ const raw = params?.[0]
+ if (typeof raw !== "string") {
+ response.result = 400
+ response.response = null
+ response.extra = { error: "Invalid username type" }
+ break
+ }
+ const normalized = raw.trim().replace(/^@/, "").toLowerCase()
+ if (!normalized) {
+ response.result = 400
+ response.response = null
+ response.extra = { error: "No username specified" }
+ break
+ }
+ // Telegram usernames: 5–32 chars, letters/numbers/underscore
+ if (!/^[a-z0-9_]{5,32}$/.test(normalized)) {
+ response.result = 400
+ response.response = null
+ response.extra = { error: "Invalid Telegram username format" }
+ break
+ }
+ response.response = await GCR.getAccountByTelegramUsername(normalized)
break
}Note: For full correctness, also make the DB lookup case‑insensitive in GCR.getAccountByTelegramUsername() (e.g., lower(telegram_id->>'username') = :username) and normalize at write-time. I can draft that change if helpful.
📝 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.
| case "getAccountByTelegramUsername": { | |
| const username = params[0] | |
| if (!username) { | |
| response.result = 400 | |
| response.response = "No username specified" | |
| break | |
| } | |
| response.response = await GCR.getAccountByTelegramUsername(username) | |
| break | |
| } | |
| case "getAccountByTelegramUsername": { | |
| const raw = params?.[0] | |
| if (typeof raw !== "string") { | |
| response.result = 400 | |
| response.response = null | |
| response.extra = { error: "Invalid username type" } | |
| break | |
| } | |
| const normalized = raw.trim().replace(/^@/, "").toLowerCase() | |
| if (!normalized) { | |
| response.result = 400 | |
| response.response = null | |
| response.extra = { error: "No username specified" } | |
| break | |
| } | |
| // Telegram usernames: 5–32 chars, letters/numbers/underscore | |
| if (!/^[a-z0-9_]{5,32}$/.test(normalized)) { | |
| response.result = 400 | |
| response.response = null | |
| response.extra = { error: "Invalid Telegram username format" } | |
| break | |
| } | |
| response.response = await GCR.getAccountByTelegramUsername(normalized) | |
| break | |
| } |
🤖 Prompt for AI Agents
In src/libs/network/manageGCRRoutines.ts around lines 86 to 97, normalize and
validate the Telegram username before calling GCR: accept strings with or
without a leading "@", trim whitespace, convert to lowercase, and ensure the
type and format are valid (non-empty alphanumeric/underscore per Telegram
rules); if invalid, set response.result = 400, response.response = null and
response.extra = { error: "<message>" } to match the handler pattern used on
lines 76–79; on success call GCR.getAccountByTelegramUsername() with the
normalized username (no "@") so DB lookups are consistent.
- Change brittle node_modules import to proper SDK export path - Update from node_modules/@kynesyslabs/demosdk/build/types/abstraction - Update to @kynesyslabs/demosdk/abstraction - Requires SDK v2.4.9+ with TelegramAttestationPayload exports - Resolves CodeRabbit security concern about import path brittleness 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…tructure initialization **Root Cause**: Partial socialAccounts objects causing undefined property access **Impact**: undefined <= 0 evaluates to false, allowing negative point deductions **Changes Made**: 1. **Fix getUserPointsInternal** (lines 114-119): - Ensure individual social account properties are always initialized - Change from object fallback to property-level null coalescing - Prevents undefined socialAccount properties 2. **Fix addPointsToGCR** (lines 193-198): - Add complete breakdown structure initialization before assignment - Prevents "Cannot set property of undefined" crashes - Ensures consistent data structure across all accounts 3. **Add defensive checks** (lines 577, 657, 821): - Twitter deduction: Extract currentTwitter with null coalescing - GitHub deduction: Extract currentGithub with null coalescing - Telegram deduction: Extract currentTelegram with null coalescing - Prevents undefined <= 0 logic errors **Security Impact**: Fixes data integrity issues preventing account corruption from negative points 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
This PR modifies Files that will be reverted:
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
run (1)
12-17: Ctrl-C handler stops the wrong compose projectYou start Compose from a per-port folder (
postgres_${PG_PORT}) butctrl_ccd’s intopostgres/. On interrupt, the DB for the running port may keep running.function ctrl_c() { HAS_BEEN_INTERRUPTED=true - cd postgres - docker compose down - cd .. + local folder="postgres_${PG_PORT}" + if [ -d "$folder" ]; then + (cd "$folder" && docker compose down) + elif [ -d "postgres" ]; then + (cd "postgres" && docker compose down) + fi }src/features/incentive/PointSystem.ts (2)
323-331: Wallet points are awarded even when already linked or chain existsYou call addPointsToGCR unconditionally, then return 400 with pointsAwarded: 0. This still increments totals.
- // Award points by updating the GCR - await this.addPointsToGCR( - userId, - pointValues.LINK_WEB3_WALLET, - "web3Wallets", - chain, - referralCode, - ) + // Award only if eligible + if (!walletIsAlreadyLinked && !hasExistingWalletOnChain) { + await this.addPointsToGCR( + userId, + pointValues.LINK_WEB3_WALLET, + "web3Wallets", + chain, + referralCode, + ) + } // Get updated points const updatedPoints = await this.getUserPointsInternal(userId)Also applies to: 336-351
200-221: Consider atomic increments to avoid lost updatesManual read-modify-write on totals/breakdowns can race. Prefer repository.increment or a transactional UPDATE ... SET col = col + ?.
♻️ Duplicate comments (3)
src/libs/abstraction/index.ts (3)
7-10: Good: types now imported from SDK public APIThis resolves the brittle import-from-build issue flagged earlier.
24-68: Authorize-bot check: cache results and make balance parsing robustAvoid dynamic import and full scan on every call; normalize and cache authorized bot addresses; treat balances as number/string/bigint. This cuts latency and prevents false negatives on non-zero forms.
-async function checkBotAuthorization(botAddress: string): Promise<boolean> { +const AUTHORIZED_BOTS = new Set<string>() +let authInit = false +async function checkBotAuthorization(botAddress: string): Promise<boolean> { try { - // Import Chain class to access genesis block - const chainModule = (await import("@/libs/blockchain/chain")).default - - // Get the genesis block (block number 0) - const genesisBlock = await chainModule.getGenesisBlock() + if (!authInit) { + const chainModule = (await import("@/libs/blockchain/chain")).default + const genesisBlock = await chainModule.getGenesisBlock() if (!genesisBlock || !genesisBlock.content) { console.error("Genesis block not found or has no content") return false } - // REVIEW It should be ok but if something goes wrong check if the genesis block returned structure is correct - // NOTE We might want to typize the genesis block return, in case - // Check if the bot address exists in genesis block balances - // Genesis block content should contain the initial address balances - const balances = genesisBlock.content.balances || genesisBlock.content - - // Check if bot address exists in balances (array of [address, balance] tuples) - if (balances && Array.isArray(balances)) { - const normalizedBotAddress = botAddress.toLowerCase() - - // Check if address exists in balances array - for (const balanceEntry of balances) { - if (Array.isArray(balanceEntry) && balanceEntry.length >= 2) { - const [address, balance] = balanceEntry - if ( - typeof address === "string" && - address.toLowerCase() === normalizedBotAddress - ) { - // Bot found in genesis with non-zero balance - authorized - return balance !== "0" && balance !== 0 - } - } - } - } - // Bot address not found in genesis balances - unauthorized - return false + const raw = genesisBlock.content.balances ?? genesisBlock.content + const balances: Array<[string, unknown]> = Array.isArray(raw) ? raw : [] + for (const entry of balances) { + if (!Array.isArray(entry) || entry.length < 2) continue + const [address, bal] = entry + if (typeof address !== "string") continue + const nonZero = + typeof bal === "bigint" ? bal !== 0n : + typeof bal === "number" ? bal !== 0 : + typeof bal === "string" ? bal.trim() !== "" && bal !== "0" && bal !== "0x0" : + Boolean(bal) + if (nonZero) AUTHORIZED_BOTS.add(address.toLowerCase()) + } + authInit = true + } + return AUTHORIZED_BOTS.has(botAddress.toLowerCase()) } catch (error) { console.error(`Bot authorization check failed: ${error}`) return false } }
112-123: Use canonical serialization and restrict accepted algorithmsJSON.stringify is not canonical; key order differences can break signatures. Also reject unexpected algorithms (accept ED25519 only).
- const messageToVerify = JSON.stringify(attestationPayload) + const messageToVerify = canonicalStringify(attestationPayload) @@ - const botSignatureValid = await ucrypto.verify({ - algorithm: signature.type, + const allowed = new Set(["ED25519"]) + if (!allowed.has(signature.type)) { + return { success: false, message: "Unsupported signature algorithm" } + } + const botSignatureValid = await ucrypto.verify({ + algorithm: signature.type, message: new TextEncoder().encode(messageToVerify), publicKey: hexToUint8Array(botAddress), // Bot's public key signature: hexToUint8Array(signature.data), // Bot signature }) + function canonicalStringify(obj: unknown): string { + if (obj === null || typeof obj !== "object") return JSON.stringify(obj) + if (Array.isArray(obj)) return `[${obj.map(canonicalStringify).join(",")}]` + const entries = Object.entries(obj as Record<string, unknown>).sort(([a],[b]) => a.localeCompare(b)) + return `{${entries.map(([k,v]) => `${JSON.stringify(k)}:${canonicalStringify(v)}`).join(",")}}` + }
🧹 Nitpick comments (5)
run (1)
132-137: Nit: update comment to reflect BunComment still mentions yarn.
-# PG_PORT and IDENTITY_FILE are exported for both docker and yarn +# PG_PORT and IDENTITY_FILE are exported for both docker and Bunsrc/libs/abstraction/index.ts (2)
102-110: Normalize hex before comparing public keysMismatch in 0x‑prefix/casing can cause false rejections. Normalize both sides.
- if (userPublicKey.toLowerCase() !== sender.toLowerCase()) { + const norm = (h: string) => h.toLowerCase().replace(/^0x/,'') + if (norm(userPublicKey) !== norm(sender)) { return { success: false, message: "Telegram attestation public key does not match transaction sender", } }
76-85: Strengthen attestation type checksValidate
signature.type/signature.datapresence and types to avoid runtime errors.- if (!telegramAttestation.payload || !telegramAttestation.signature) { + if ( + !telegramAttestation.payload || + !telegramAttestation.signature || + typeof telegramAttestation.signature.type !== "string" || + typeof telegramAttestation.signature.data !== "string" + ) { return { success: false, message: "Invalid telegram attestation format", } }src/features/incentive/PointSystem.ts (2)
794-866: Telegram deduction flow: prevent negative totalsConsider clamping per‑platform and total points at zero after deduction to avoid negative balances on repeated calls or drift.
await this.addPointsToGCR( userId, -pointValues.LINK_TELEGRAM, "socialAccounts", "telegram", ) const updatedPoints = await this.getUserPointsInternal(userId) + // Optionally clamp in persistence layer to avoid negatives (follow-up change) + // await this.clampNonNegative(userId, ["totalPoints","breakdown.socialAccounts.telegram"])
35-38: Expose Telegram in linkedSocials returned by getUserIdentitiesFromGCRAPI shape currently only exposes twitter. Include telegram username/id for parity.
- private async getUserIdentitiesFromGCR(userId: string): Promise<{ - linkedWallets: string[] - linkedSocials: { twitter?: string } - }> { + private async getUserIdentitiesFromGCR(userId: string): Promise<{ + linkedWallets: string[] + linkedSocials: { twitter?: string; telegram?: string } + }> { @@ - const linkedSocials: { twitter?: string } = {} + const linkedSocials: { twitter?: string; telegram?: string } = {} @@ + const telegramIdentities = await IdentityManager.getWeb2Identities( + userId, + "telegram", + ) + if (Array.isArray(telegramIdentities) && telegramIdentities.length > 0) { + linkedSocials.telegram = telegramIdentities[0].username + }Also applies to: 67-73
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
TO_FIX.md(1 hunks)run(3 hunks)src/features/incentive/PointSystem.ts(6 hunks)src/libs/abstraction/index.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- TO_FIX.md
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/incentive/PointSystem.ts (1)
src/libs/blockchain/gcr/gcr_routines/ensureGCRForUser.ts (1)
ensureGCRForUser(8-33)
🔇 Additional comments (5)
src/features/incentive/PointSystem.ts (4)
16-16: Point value for Telegram addedLooks good and consistent with Twitter.
114-119: Good defaults for socialAccounts including TelegramSafe nullish coalescing prevents undefined math.
193-199: Initialize breakdown defensivelyPrevents NPEs when older rows lack breakdown; includes telegram.
203-211: Platform allowlist includes TelegramUpdate is correct and consistent with later access.
run (1)
170-172: Container name present in compose; no change required
docker-compose.yml (services.db) sets container_name: postgres_${PG_PORT}, so docker exec postgres_${PG_PORT} ... is valid.
|
This PR modifies Files that will be reverted:
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (17)
.gitignore (3)
138-138: Generalize CLAUDE conflict artifact ignore.Use a wildcard so future conflict files are also ignored.
-CLAUDE.sync-conflict-20250901-171031-7JPPSQB.md +CLAUDE.sync-conflict-*.md
129-129: Consolidate Serena artifacts ignore.Single-file ignores are brittle. Ignore the whole Serena cache/backup area.
-.serena-backup -.serena/cache/typescript/document_symbols_cache_v23-06-25.pkl +.serena/If you need some tracked subpaths under
.serena/, add negation rules for those explicitly.Also applies to: 139-139
136-137: Reconsider ignoring contract docs.
GASLESS_BRIDGE_FLOW_DIAGRAM.mdandUSAGE.mdlook like source docs worth versioning. Suggest keeping them tracked (or moving to/documentation/), not ignored.-src/features/bridges/EVMSmartContract/GASLESS_BRIDGE_FLOW_DIAGRAM.md -src/features/bridges/EVMSmartContract/USAGE.md.serena/memories/pr_review_import_fix_completed.md (1)
10-10: Fix markdownlint MD026 (no trailing punctuation in headings).Remove trailing colons to satisfy markdownlint.
-### Resolution Steps Taken: +### Resolution Steps Taken -### Code Changes: +### Code Changes -### Next Critical Issues to Address: +### Next Critical Issues to Address -### Validation Required: +### Validation RequiredAlso applies to: 15-15, 30-30, 35-35
.serena/memories/pr_review_all_high_priority_completed.md (1)
12-12: Fix markdownlint MD026 (heading punctuation).Drop trailing colons from headings.
-### CRITICAL Issues (Phase 1) - ALL COMPLETED: +### CRITICAL Issues (Phase 1) - ALL COMPLETED -### HIGH Priority Issues (Phase 2) - ALL COMPLETED: +### HIGH Priority Issues (Phase 2) - ALL COMPLETED -### Key Technical Accomplishments: +### Key Technical Accomplishments -### Architecture Insights Discovered: +### Architecture Insights Discovered -### Success Criteria Status: +### Success Criteria StatusAlso applies to: 18-18, 23-23, 39-39, 49-49
.serena/memories/pr_review_json_canonicalization_dismissed.md (2)
20-21: Correct RFC reference: use RFC 8785 (JCS), not RFC 7515.RFC 8785 defines JSON Canonicalization Scheme; RFC 7515 is JWS.
-- RFC 7515 JCS standard would be better than custom implementation +- RFC 8785 (JSON Canonicalization Scheme, JCS) would be better than a custom implementation
10-10: Fix markdownlint MD026 (heading punctuation).-### Analysis: +### Analysis -### Technical Issues with Proposed Fix: +### Technical Issues with Proposed Fix -### Current Status: +### Current Status -### Updated Critical Issues Count: +### Updated Critical Issues CountAlso applies to: 16-16, 22-22, 25-25
.serena/memories/session_checkpoint_2025_01_31.md (1)
17-17: Fix markdownlint MD026 (heading punctuation).-## Final Session Summary: +## Final Session Summary.serena/memories/genesis_caching_security_dismissed.md (2)
11-11: Fix markdownlint MD026 (heading punctuation).-### Security Analysis: +### Security Analysis -### Current Implementation Security Benefits: +### Current Implementation Security Benefits -### Performance vs Security Trade-off: +### Performance vs Security Trade-off -### Updated Priority Assessment: +### Updated Priority Assessment -### Next Focus Areas: +### Next Focus AreasAlso applies to: 18-18, 24-24, 29-29, 35-35
24-28: Instrument and bound live validation cost.Add telemetry (timers, error rates), timeouts, and fallback behavior for genesis fetch; document SLO/latency budget and backoff to mitigate upstream slowness.
.serena/memories/pr_review_point_system_fixes_completed.md (1)
12-12: Fix markdownlint MD026 (heading punctuation).-### Critical Issue #4: Point System Null Pointer Bug +### Critical Issue #4: Point System Null Pointer Bug -### Root Cause Analysis: +### Root Cause Analysis -### Comprehensive Solution Implemented: +### Comprehensive Solution Implemented -### Next Priority Issues: +### Next Priority Issues -### Validation Status: +### Validation StatusAlso applies to: 18-18, 23-23, 39-39, 49-49
.serena/memories/data_structure_robustness_completed.md (1)
11-11: Fix markdownlint MD026 (heading punctuation).-### Implementation Details: +### Implementation Details -```typescript +```typescript -### Root Cause Analysis: +### Root Cause Analysis -### Integration with Previous Fixes: +### Integration with Previous Fixes -### Updated HIGH Priority Status: +### Updated HIGH Priority StatusAlso applies to: 18-18, 24-24, 29-29, 35-35
.serena/memories/input_validation_improvements_completed.md (2)
10-10: Fix markdownlint MD026 (heading punctuation).-### Security-First Implementation: +### Security-First Implementation -### Changes Made: +### Changes Made -### Security Benefits: +### Security Benefits -### Compatibility: +### Compatibility -### ALL HIGH Priority Issues Now Complete: +### ALL HIGH Priority Issues Now CompleteAlso applies to: 13-13, 61-61, 67-67, 72-72
44-59: Avoid echoing sensitive identifiers in error messages.Consider gating “expected vs got” IDs/usernames behind a debug flag or logs to reduce info exposure to clients.
- message: `Telegram ID mismatch: expected ${payloadId}, got ${attestationId}`, + message: "Telegram ID mismatch",.serena/memories/session_final_checkpoint_2025_01_31.md (3)
12-12: Fix markdownlint MD026: remove trailing colons in headingsRemove trailing ":" from headings to satisfy markdownlint.
-### Major Accomplishments This Session: +### Major Accomplishments This Session -### Complete Issue Resolution Summary: +### Complete Issue Resolution Summary -#### CRITICAL Issues (4/4 Complete): +#### CRITICAL Issues (4/4 Complete) -#### HIGH Priority Issues (3/3 Complete): +#### HIGH Priority Issues (3/3 Complete) -### Technical Achievements: +### Technical Achievements -### Session Patterns Established: +### Session Patterns Established -### Files Modified This Session: +### Files Modified This Session -### Next Available Work: +### Next Available Work -### Validation Remaining: +### Validation RemainingAlso applies to: 18-18, 20-20, 26-26, 31-31, 37-37, 43-43, 48-48, 54-54
43-47: Avoid brittle line-number references; cite symbols/commits instead“lines 86-123” will drift. Prefer function/type names and/or commit hashes or permalinks.
-- `src/libs/abstraction/index.ts` - Enhanced input validation (lines 86-123) +- `src/libs/abstraction/index.ts` — Enhanced input validation in `verifyWeb2Proof()/verifyTelegramProof()` (see commit <short-sha>)
54-59: Add exact validation commands to make this checklist actionableList the concrete commands CI/devs should run.
### Validation Remaining - Security verification passes - All tests pass with linting - Type checking passes with `bun tsc --noEmit` +Recommended commands: +``` +bun test +bun run lint +bun tsc --noEmit +```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
.gitignore(1 hunks).serena/memories/data_structure_robustness_completed.md(1 hunks).serena/memories/genesis_caching_security_dismissed.md(1 hunks).serena/memories/input_validation_improvements_completed.md(1 hunks).serena/memories/pr_review_all_high_priority_completed.md(1 hunks).serena/memories/pr_review_analysis_complete.md(1 hunks).serena/memories/pr_review_corrected_analysis.md(1 hunks).serena/memories/pr_review_import_fix_completed.md(1 hunks).serena/memories/pr_review_json_canonicalization_dismissed.md(1 hunks).serena/memories/pr_review_point_system_fixes_completed.md(1 hunks).serena/memories/project_patterns_telegram_identity_system.md(1 hunks).serena/memories/session_checkpoint_2025_01_31.md(1 hunks).serena/memories/session_final_checkpoint_2025_01_31.md(1 hunks).serena/memories/session_pr_review_completion_2025_01_31.md(1 hunks)TO_FIX.md(1 hunks)package.json(1 hunks)src/libs/abstraction/index.ts(3 hunks)
✅ Files skipped from review due to trivial changes (4)
- .serena/memories/pr_review_corrected_analysis.md
- .serena/memories/session_pr_review_completion_2025_01_31.md
- .serena/memories/project_patterns_telegram_identity_system.md
- TO_FIX.md
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/libs/abstraction/index.ts
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
.serena/memories/pr_review_point_system_fixes_completed.md
12-12: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
18-18: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
23-23: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
39-39: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
49-49: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
.serena/memories/input_validation_improvements_completed.md
10-10: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
13-13: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
61-61: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
67-67: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
72-72: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
.serena/memories/data_structure_robustness_completed.md
11-11: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
18-18: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
24-24: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
29-29: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
35-35: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
.serena/memories/genesis_caching_security_dismissed.md
11-11: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
18-18: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
24-24: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
29-29: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
35-35: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
.serena/memories/pr_review_all_high_priority_completed.md
12-12: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
18-18: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
23-23: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
39-39: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
49-49: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
.serena/memories/pr_review_import_fix_completed.md
10-10: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
15-15: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
30-30: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
35-35: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
.serena/memories/pr_review_json_canonicalization_dismissed.md
10-10: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
16-16: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
22-22: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
25-25: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
.serena/memories/session_checkpoint_2025_01_31.md
17-17: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
.serena/memories/session_final_checkpoint_2025_01_31.md
12-12: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
18-18: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
20-20: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
26-26: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
31-31: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
37-37: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
43-43: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
48-48: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
54-54: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (12)
.gitignore (4)
131-134: LGTM: build outputs correctly ignored.
artifacts/,cache/, andout/are generated by Hardhat/Foundry; safe to ignore.
133-133: Verify ignoringlib/for Foundry.Foundry’s
lib/often contains vendored deps. If you rely on vendoring (no lockfile/submodules), don’t ignore it. If deps are reproducibly fetched on setup, keeping it ignored is fine.
130-130: Confirmdist/policy for packages.If any published package consumes
dist/, ensure CI publishes artifacts orfilesin package.json includes built outputs; otherwise releases may miss code.
141-141: LGTM: ignore automation scratch dir.
PR_COMMENTS/as ignore is appropriate for workflow artifacts..serena/memories/pr_review_import_fix_completed.md (1)
12-13: SDK version mismatch: v2.4.7 vs v2.4.9.PR objectives state v2.4.7, this doc states v2.4.9. Align and reference the exact published version used by the repo.
Apply one of:
-2. **SDK Published**: Version 2.4.9 published with proper exports +2. **SDK Published**: Version 2.4.7 published with proper exportsor:
-2. **SDK Published**: Version 2.4.9 published with proper exports +2. **SDK Published**: Version 2.4.9 published with proper exports (repo updated accordingly).serena/memories/pr_review_all_high_priority_completed.md (1)
40-41: Substantiate “Addresses ARE Ed25519 public keys.”Add a brief reference (spec, code location, or test) confirming address = raw Ed25519 pubkey to avoid future regressions.
.serena/memories/session_checkpoint_2025_01_31.md (1)
37-39: Inconsistency: “Genesis caching opportunity” vs. dismissed security risk.Other docs mark genesis caching as DISMISSED. Update this section to reflect the decision.
-- `src/libs/abstraction/index.ts` - Genesis caching opportunity (line 24-68) +- `src/libs/abstraction/index.ts` - Genesis caching (DISMISSED as security risk) — keep live validation.serena/memories/pr_review_point_system_fixes_completed.md (2)
62-66: Inconsistency: lists “Genesis block caching optimization” as next priority.Elsewhere caching is dismissed. Update to match the security decision.
-**HIGH Priority (Performance & Stability)**: -- Genesis block caching optimization +**HIGH Priority (Performance & Stability)**: +// Genesis block caching — DISMISSED (security risk)
7-8: LINK_TELEGRAM = 2 — verified; no change required.
Declared and used consistently in src/features/incentive/PointSystem.ts (lines 16, 195, 757, 768, 838, 848)..serena/memories/data_structure_robustness_completed.md (1)
6-12: Keep line references accurate.Confirm the cited lines (193-198) still match PointSystem.ts after recent edits to prevent stale pointers in docs.
.serena/memories/pr_review_analysis_complete.md (1)
49-53: Align Phase 2 entry with security guidance for genesis cachingIf "Implement genesis block caching" is considered a security risk, do not list it in Phase 2 unless concrete mitigations are added (authentication, content pinning, input/consistency validation); otherwise remove or reclassify it so the docs are consistent.
Location: .serena/memories/pr_review_analysis_complete.md lines 49–53
Repo search returned no results in the sandbox—confirm the file contents and apply one of the above fixes.
.serena/memories/session_final_checkpoint_2025_01_31.md (1)
20-29: Unify stance on JSON canonicalization and genesis caching — add ADR/issueCheckpoint (.serena) marks JSON canonicalization as a FALSE POSITIVE and genesis caching as dismissed, while the PR treats canonicalization as CRITICAL and proposes adding genesis caching. Reconcile into a single, traceable decision (ADR/issue).
Evidence — signature/hash codepaths that will be affected if serialization changes:
- src/libs/network/endpointHandlers.ts:224–227 — Hashing.sha256(JSON.stringify(validatedData.data)) used before ucrypto.verify.
- src/libs/blockchain/gcr/gcr_routines/registerIMPData.ts:22–26 — Hashing.sha256(JSON.stringify(message.message)) then Cryptography.verify(...).
- src/libs/communications/transmission.ts:65 — bundle.hash = Hashing.sha256(JSON.stringify(this.bundle.content)). (comment: "REVIEW is this ok?")
- src/libs/peer/routines/peerGossip.ts:56 — peersHash = Hashing.sha256(JSON.stringify(orderedPeers)) (gossip/consensus).
- src/libs/blockchain/transaction.ts:221–249 — transaction signature verification depends on tx.hash/content serialization.
Evidence — genesis / bot-authorization paths:
- src/libs/abstraction/index.ts:24–48,70–76,142–160 — checkBotAuthorization() reads genesis balances; verifyTelegramProof() validates bot signature and calls checkBotAuthorization; genesis caching decision directly impacts bot authorization logic.
Action required:
- Add an ADR/issue that records the chosen approach.
- If adopting canonicalization: implement a canonical serializer and update all signing/verification codepaths above; provide a migration plan and tests.
- If keeping current JSON.stringify behavior: centralize the serializer, add deterministic unit tests, and document the rationale and risks in the ADR.
| src/features/bridges/EVMSmartContract/cache | ||
| src/features/bridges/EVMSmartContract/lib | ||
| src/features/bridges/EVMSmartContract/out | ||
| src/features/bridges/EVMSmartContract/test |
There was a problem hiding this comment.
Do not ignore smart‑contract tests.
Ignoring src/features/bridges/EVMSmartContract/test will prevent future test changes from being tracked. Keep tests versioned; only ignore build artifacts.
Apply this diff:
-src/features/bridges/EVMSmartContract/test📝 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.
| src/features/bridges/EVMSmartContract/test |
🤖 Prompt for AI Agents
In .gitignore around line 135, the entry
"src/features/bridges/EVMSmartContract/test" is ignoring the smart‑contract test
directory; remove that line so test files are tracked, and if you need to ignore
build artifacts instead, add specific artifact patterns (e.g., build/dist
folders or compiled files) rather than the test directory.
| **1. Type Validation (Security Layer)**: | ||
| ```typescript | ||
| // Validate attestation data types first (trusted source should have proper format) | ||
| if (typeof telegramAttestation.payload.telegram_id !== 'number' && | ||
| typeof telegramAttestation.payload.telegram_id !== 'string') { | ||
| return { | ||
| success: false, | ||
| message: "Invalid telegram_id type in bot attestation", | ||
| } | ||
| } | ||
|
|
||
| if (typeof telegramAttestation.payload.username !== 'string') { | ||
| return { | ||
| success: false, | ||
| message: "Invalid username type in bot attestation", | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **2. Safe Normalization (After Type Validation)**: | ||
| ```typescript | ||
| // Safe type conversion and normalization | ||
| const attestationId = telegramAttestation.payload.telegram_id.toString() | ||
| const payloadId = payload.userId?.toString() || '' | ||
|
|
||
| const attestationUsername = telegramAttestation.payload.username.toLowerCase().trim() | ||
| const payloadUsername = payload.username?.toLowerCase()?.trim() || '' | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deduplicate normalization/validation.
Extract shared Telegram attestation normalization into a single helper to prevent drift across call sites.
🤖 Prompt for AI Agents
.serena/memories/input_validation_improvements_completed.md around lines 15 to
42: the duplicate type checks and normalization for telegram attestation should
be extracted into one reusable helper to avoid drift; implement a helper (e.g.,
normalizeTelegramAttestation) that accepts the attestation and optional payload,
performs the type validation (ensuring telegram_id is number|string and username
is string), does safe conversions (toString(), toLowerCase(), trim()) and
returns a standardized object or throws/returns an error result, then replace
the inline checks and conversions at all call sites with calls to this helper
and handle its standardized success/error return consistently.
| ### 1. Bot Signature Verification Flaw (CRITICAL) | ||
| - **Location**: `src/libs/abstraction/index.ts:117-123` | ||
| - **Problem**: Using `botAddress` as public key for signature verification | ||
| - **Risk**: Authentication bypass - addresses ≠ public keys | ||
| - **Status**: Must fix immediately | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Be precise about the verification flaw and remediation path
Call out the exact symbols/files (e.g., verifyTelegramProof, key source, curve/algorithm), and confirm that verification uses the bot’s public key, not its address.
🏁 Script executed:
#!/bin/bash
# Locate verification code, key sources, and any 'address as pubkey' misuse
rg -nP --type=ts -C3 'verifyTelegramProof|verifyWeb2Proof|bot(Address|Key)|public key|secp|ed25519|verify\(' src/Length of output: 103191
CRITICAL — verifyTelegramProof uses bot address as the verification key; replace with the bot’s ed25519 public key
verifyTelegramProof (src/libs/abstraction/index.ts) destructures attestationPayload.bot_address and calls ucrypto.verify(..., publicKey: hexToUint8Array(botAddress), ...). checkBotAuthorization(botAddress) treats the same value as an on‑chain address (balances lookup). Addresses are not raw Ed25519 public keys — using bot_address here enables signature verification bypass or verification against attacker-controlled data.
- Fix: stop using bot_address as the verifier key. Require or retrieve the bot’s authoritative ed25519 public key and pass that to ucrypto.verify (e.g. add/require attestationPayload.bot_public_key or fetch the bot pubkey from on‑chain GCR/genesis/trusted mapping).
- Harden: verify signature.algorithm == "ed25519" and that publicKey decodes to a 32‑byte buffer before calling ucrypto.verify.
- Keep checkBotAuthorization as an authorization/balance check only — do not use it as the source of the verification key.
Locations: src/libs/abstraction/index.ts — checkBotAuthorization (≈lines 24–68) and verifyTelegramProof (≈lines 70–150; ucrypto.verify call uses publicKey: hexToUint8Array(botAddress)).
🤖 Prompt for AI Agents
In .serena/memories/pr_review_analysis_complete.md around lines 17 to 22, the
review notes that verifyTelegramProof incorrectly uses bot_address (an on‑chain
address) as the Ed25519 public key for ucrypto.verify, which enables signature
verification bypass; change verifyTelegramProof to require or fetch the bot's
authoritative ed25519 public key (e.g., add attestationPayload.bot_public_key or
retrieve it from the trusted on‑chain mapping/GCR/genesis) and pass that to
ucrypto.verify instead of bot_address, validate signature.algorithm ===
"ed25519", decode the provided public key and assert it is exactly 32 bytes
before calling ucrypto.verify, and keep checkBotAuthorization purely for
authorization/balance checks (do NOT derive the verification key from
bot_address).
| ### 2. JSON Canonicalization Missing (CRITICAL) | ||
| - **Location**: `src/libs/abstraction/index.ts` | ||
| - **Problem**: Non-deterministic JSON.stringify() for signature verification | ||
| - **Risk**: Intermittent signature failures | ||
| - **Status**: Must implement canonical serialization | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Contradiction: canonical JSON marked CRITICAL here but dismissed in the session checkpoint
Unify the decision: either adopt canonical serialization (and specify library/algorithm) or document why it’s unsafe/unnecessary. Link to an ADR and update both docs accordingly.
🏁 Script executed:
#!/bin/bash
# Cross-check all mentions to decide the single source of truth
rg -n --glob '.serena/**' -e 'canonical|canonicalization'
# Identify actual serializers used in code
rg -nP --type=ts -C2 '\b(JSON\.stringify|canonical|stable-stringify)\b' src/Length of output: 62576
Critical: adopt canonical JSON (RFC‑8785/JCS) for all hashing/signing or document an explicit exception (ADR + docs)
JSON.stringify is used in multiple cryptographic paths and will produce non-deterministic hashes/signatures across nodes. Replace these uses with an RFC‑8785-compliant canonicalizer (or document why canonicalization is unnecessary) and record the decision in an ADR.
Critical locations to fix: src/libs/abstraction/index.ts (messageToVerify), src/libs/blockchain/transaction.ts (tx hash/sign), src/libs/communications/transmission.ts (bundle.hash), src/libs/network/endpointHandlers.ts (gcrEdits / tx.content / validationData), src/libs/blockchain/routines/validateTransaction.ts, src/libs/blockchain/gcr/gcr_routines/hashGCR.ts, src/libs/l2ps/parallelNetworks.ts, src/libs/blockchain/chain.ts (genesis hashes), src/libs/utils/demostdlib/deriveMempoolOperation.ts.
Action items: choose and declare an RFC‑8785-compliant library/algorithm, replace JSON.stringify(...) in the listed (and any other crypto) paths with the canonicalizer, add deterministic-hash tests, update ADR/docs and include a migration/compatibility plan.
🤖 Prompt for AI Agents
In .serena/memories/pr_review_analysis_complete.md around lines 23-28: the
review flags non-deterministic JSON.stringify usage affecting cryptographic
hashes/signatures; replace all JSON.stringify(...) used in crypto paths
(starting with src/libs/abstraction/index.ts -> messageToVerify) with an
RFC-8785/JCS-compliant canonicalizer library (pick and import a maintained
implementation), update each affected file to call the canonicalize(...) output
when hashing/signing/verifying, add deterministic-hash unit tests covering those
paths, and record the chosen canonicalization decision and
migration/compatibility notes in an ADR and docs.
| - ✅ `TO_FIX.md` - Comprehensive fix tracking document | ||
| - ✅ References to all comment files in `PR_COMMENTS/review-3222019024-comments/` | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure referenced comment artifacts exist
Verify that PR_COMMENTS/review-3222019024-comments/ and TO_FIX.md are present to avoid dead references.
🏁 Script executed:
#!/bin/bash
fd -t d -H 'PR_COMMENTS' .
fd -t d -H 'review-3222019024-comments' .
fd -t f -H 'TO_FIX.md' .Length of output: 11
Missing PR_COMMENTS/review-3222019024-comments — add or update reference
TO_FIX.md exists at ./TO_FIX.md; PR_COMMENTS/review-3222019024-comments/ was not found. Update .serena/memories/pr_review_analysis_complete.md (lines 60–62) to remove or correct the dead reference, or add the missing directory with the referenced comment files.
🤖 Prompt for AI Agents
.serena/memories/pr_review_analysis_complete.md around lines 60 to 62 references
a non-existent PR_COMMENTS/review-3222019024-comments directory; either remove
or correct that dead reference or create the missing directory with the
referenced comment files. Edit the file to delete or update the reference to the
correct path (or add a brief note if intentionally omitted), or create
PR_COMMENTS/review-3222019024-comments/ and add the expected comment files, then
stage and commit the change so the TO_FIX.md and references are consistent.
| ### Current Status: | ||
| ✅ **NO ACTION REQUIRED** - Existing JSON.stringify implementation works reliably for simple flat objects | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Caveat: JSON.stringify is not a formal canonicalization.
If signatures depend on key order, document that the signing input is a deterministic field list, not generic stringify of arbitrary objects.
-✅ **NO ACTION REQUIRED** - Existing JSON.stringify implementation works reliably for simple flat objects
+✅ **NO ACTION REQUIRED** (for now) — Ensure signatures are produced from an explicit, ordered field list to avoid engine-dependent serialization differences.📝 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.
| ### Current Status: | |
| ✅ **NO ACTION REQUIRED** - Existing JSON.stringify implementation works reliably for simple flat objects | |
| ### Current Status: | |
| ✅ **NO ACTION REQUIRED** (for now) — Ensure signatures are produced from an explicit, ordered field list to avoid engine-dependent serialization differences. |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
22-22: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🤖 Prompt for AI Agents
.serena/memories/pr_review_json_canonicalization_dismissed.md around lines
22-24: the file currently claims JSON.stringify is sufficient but fails to state
that it is not a formal canonicalization; update the note to explicitly document
that if any signatures depend on key order the codebase expects a deterministic
field list as the signing input (not a generic JSON.stringify of arbitrary
objects) and either (a) list the exact deterministic field ordering used for
signing or (b) state that callers must canonicalize (e.g., sort keys) before
signing, so readers know the required behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/libs/abstraction/index.ts (4)
7-8: Stop importing types from node_modules internalsImporting
GenesisBlockvia a deep path is brittle. Use the SDK’s public API (request a re-export if missing) or rely on a local runtime-narrowed type instead.
32-80: Bot authorization: robustness + caching
- Dynamic import and re-scan per call; cache the authorized set once.
- Current non-zero check can misread BigInt/hex/large values.
- Avoid coupling to exact genesis shape; guard defensively.
Apply this diff (and remove the
toIntegerimport above):+const AUTHORIZED_BOTS = new Set<string>() +let authInit = false async function checkBotAuthorization(botAddress: string): Promise<boolean> { try { - // Import Chain class and GenesisBlock type to access genesis block - const chainModule = (await import("@/libs/blockchain/chain")).default + if (!authInit) { + const chainModule = (await import("@/libs/blockchain/chain")).default - // Get the genesis block (block number 0) - const genesisBlock = - (await chainModule.getGenesisBlock()) as GenesisBlock + const genesisBlock = await chainModule.getGenesisBlock() if (!genesisBlock || !genesisBlock.content) { console.error("Genesis block not found or has no content") return false } - // REVIEW: Now properly typed - accessing genesis data from extra.genesisData - if (!genesisBlock.content.extra?.genesisData) { + if (!genesisBlock.content.extra?.genesisData) { console.error("Genesis block missing extra.genesisData") return false } - // Get balances from properly typed genesis data - const balances = genesisBlock.content.extra.genesisData.balances + const balances = genesisBlock.content.extra.genesisData.balances if (!balances || !Array.isArray(balances)) { console.error("Genesis block balances not found or invalid format") return false } - const normalizedBotAddress = botAddress.toLowerCase() - // Check if bot address exists in balances array - for (const [address, balance] of balances) { - if ( - typeof address === "string" && - address.toLowerCase() === normalizedBotAddress - ) { - // Bot found in genesis with non-zero balance - authorized - return balance !== "0" && toInteger(balance) !== 0 - } - } - // Bot address not found in genesis balances - unauthorized - return false + for (const entry of balances) { + if (!Array.isArray(entry) || entry.length < 2) continue + const [address, bal] = entry + if (typeof address !== "string") continue + const nonZero = + typeof bal === "bigint" ? bal !== 0n : + typeof bal === "number" ? bal !== 0 : + typeof bal === "string" ? bal.trim() !== "" && bal !== "0" && bal !== "0x0" : + Boolean(bal) + if (nonZero) AUTHORIZED_BOTS.add(address.toLowerCase()) + } + authInit = true + } + return AUTHORIZED_BOTS.has(botAddress.toLowerCase()) } catch (error) { console.error(`Bot authorization check failed: ${error}`) return false } }
156-167: Use canonical JSON + algorithm allowlist + key format checks
- JSON.stringify is non-canonical; use a deterministic canonical string.
- Allow only expected algorithms (e.g., ED25519).
- Normalize hex (strip 0x, lower-case) and sanity-check lengths before verify.
Apply this diff:
- // Prepare the message that was signed (stringify the payload for consistent hashing) - const messageToVerify = JSON.stringify(attestationPayload) + // Prepare canonical message for signature verification + const messageToVerify = canonicalStringify(attestationPayload) @@ - const botSignatureValid = await ucrypto.verify({ - algorithm: signature.type, + const algo = signature.type + if (!ALLOWED_TELEGRAM_SIG_ALGOS.has(algo)) { + return { success: false, message: `Unsupported signature algorithm: ${algo}` } + } + const botPk = normalizeHex(botPublicKeyHex) + if (botPk.length !== 64) { + return { success: false, message: "Invalid bot public key length" } + } + const sigHex = normalizeHex(signature.data) + const botSignatureValid = await ucrypto.verify({ + algorithm: algo, message: new TextEncoder().encode(messageToVerify), - publicKey: hexToUint8Array(botAddress), // Bot's public key - signature: hexToUint8Array(signature.data), // Bot signature + publicKey: hexToUint8Array(botPk), + signature: hexToUint8Array(sigHex), })Add these helpers/constants (outside this hunk):
const ALLOWED_TELEGRAM_SIG_ALGOS = new Set(["ED25519"]) function normalizeHex(h: string): string { const s = (h || "").toLowerCase() return s.startsWith("0x") ? s.slice(2) : s } // Deterministic, key‑sorted JSON function canonicalStringify(obj: unknown): string { if (obj === null || typeof obj !== "object") return JSON.stringify(obj) if (Array.isArray(obj)) return `[${obj.map(canonicalStringify).join(",")}]` const entries = Object.entries(obj as Record<string, unknown>) .filter(([, v]) => v !== undefined) // drop undefined like JSON.stringify .sort(([a], [b]) => a.localeCompare(b)) return `{${entries .map(([k, v]) => `${JSON.stringify(k)}:${canonicalStringify(v)}`) .join(",")}}` }
184-193: Attestation freshness / replay-resistanceNo nonce/issued_at/expiry validation; attestations can be replayed indefinitely. Enforce TTL and one-time nonce per telegram_id.
Example (add after signature checks):
const nowMs = Date.now() const issuedAtMs = Date.parse((attestationPayload as any).issued_at || "") const maxSkewMs = 2 * 60 * 1000 const maxTtlMs = 10 * 60 * 1000 if (!Number.isFinite(issuedAtMs) || issuedAtMs > nowMs + maxSkewMs || nowMs - issuedAtMs > maxTtlMs) { return { success: false, message: "Attestation expired or not yet valid" } } // TODO: persist and reject reused nonce per telegram_id (e.g., Redis/DB)
🧹 Nitpick comments (4)
src/libs/abstraction/index.ts (4)
15-31: Duplicate JSDoc blocksTwo nearly identical docblocks back-to-back. Remove the duplicate to reduce noise.
175-182: Normalize before auth checkPass a normalized bot public key/addr into
checkBotAuthorizationto avoid case/0x mismatches.Apply this diff:
- const botAuthorized = await checkBotAuthorization(botAddress) + const botAuthorized = await checkBotAuthorization(normalizeHex(botPublicKeyHex))
248-252: Type guard for proofAvoid the blind cast; validate
payload.proofis a string before parsing.Apply this diff:
- const { message, type, signature } = await instance.readData( - payload.proof as string, - ) + if (typeof payload.proof !== "string") { + return { success: false, message: "Invalid proof type" } + } + const { message, type, signature } = await instance.readData(payload.proof)
141-167: Name clarity: address vs public key
bot_addressis treated as a public key. Rename locals tobotPublicKeyHexto avoid confusion.Apply this diff:
- const { public_key: userPublicKey, bot_address: botAddress } = - attestationPayload + const { public_key: userPublicKey, bot_address: botPublicKeyHex } = + attestationPayload…and update downstream references accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
package.json(1 hunks)src/libs/abstraction/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
|
This PR modifies Files that will be reverted:
|
+ update checkBotAuthorization to use new lookup + verify user signature on telegram identity submission + remove getAccountByTelegramUsername endpoint and handler + update isFirstConnection to reduce redundant web2 queries code + upgrade sdk
|
This PR modifies Files that will be reverted:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/libs/abstraction/index.ts (1)
36-42: Efficient genesis check, but should handle edge casesThe genesis identity check is now efficiently using the shared state Set. However, the function should validate the input.
Add input validation:
async function checkBotAuthorization(botAddress: string): Promise<boolean> { + if (!botAddress || typeof botAddress !== 'string') { + return false + } if (getSharedState.genesisIdentities.has(botAddress)) { return true } return false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.gitignore(2 hunks)package.json(1 hunks)run(1 hunks)src/features/incentive/PointSystem.ts(6 hunks)src/index.ts(2 hunks)src/libs/abstraction/index.ts(3 hunks)src/libs/blockchain/gcr/gcr.ts(3 hunks)src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts(15 hunks)src/libs/blockchain/gcr/gcr_routines/IncentiveManager.ts(1 hunks)src/libs/blockchain/routines/loadGenesisIdentities.ts(1 hunks)src/libs/network/manageGCRRoutines.ts(1 hunks)src/utilities/sharedState.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- run
- package.json
- .gitignore
- src/libs/blockchain/gcr/gcr_routines/IncentiveManager.ts
- src/libs/network/manageGCRRoutines.ts
- src/libs/blockchain/gcr/gcr.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/index.ts (1)
src/libs/blockchain/routines/loadGenesisIdentities.ts (1)
loadGenesisIdentities(6-18)
src/libs/abstraction/index.ts (2)
src/utilities/sharedState.ts (1)
getSharedState(262-264)src/utilities/logger.ts (1)
error(125-132)
src/libs/blockchain/routines/loadGenesisIdentities.ts (1)
src/utilities/sharedState.ts (1)
getSharedState(262-264)
src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts (4)
src/libs/abstraction/index.ts (1)
verifyWeb2Proof(173-259)src/utilities/logger.ts (1)
error(125-132)src/libs/crypto/hashing.ts (1)
Hashing(15-26)src/libs/blockchain/gcr/gcr_routines/IncentiveManager.ts (1)
IncentiveManager(9-135)
src/features/incentive/PointSystem.ts (1)
src/libs/blockchain/gcr/gcr_routines/ensureGCRForUser.ts (1)
ensureGCRForUser(8-33)
🔇 Additional comments (12)
src/utilities/sharedState.ts (1)
113-113: LGTM!The addition of
genesisIdentitiesas a Set is appropriate for efficient membership checks during bot authorization.src/index.ts (1)
273-274: LGTM!Loading genesis identities immediately after finding the genesis block ensures the data is available before any identity verification operations.
src/features/incentive/PointSystem.ts (3)
201-207: LGTM!Good defensive programming - initializing the breakdown structure ensures safe property access in subsequent operations.
839-841: LGTM!The explicit extraction of
currentTelegramwith nullish coalescing properly handles undefined values, preventing negative point balances.
722-804: Consider adding idempotency and race condition protectionThe Telegram points awarding flow correctly verifies ownership. However, concurrent requests could bypass the "already awarded" check.
Consider using database transactions or optimistic locking to prevent race conditions:
async awardTelegramPoints( userId: string, telegramUserId: string, referralCode?: string, ): Promise<RPCResponse> { try { + const db = await Datasource.getInstance() + return await db.getDataSource().transaction(async manager => { + const gcrRepo = manager.getRepository(GCRMain) // Get user's account data from GCR to verify Telegram ownership - const account = await ensureGCRForUser(userId) + const account = await gcrRepo.findOne({ + where: { pubkey: userId }, + lock: { mode: "pessimistic_write" } + }) // ... rest of the logic + }) } catch (error) {src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts (3)
702-714: Excellent refactor: Template-based query eliminates code duplicationThe generalized query template for web2 contexts is clean and maintainable, correctly handling Twitter, GitHub, Telegram, and Discord with a single code path.
1-1: Type safety concern withanycasts throughout the fileMultiple handlers use
editOperation: anywhich removes compile-time type checking. Consider using a discriminated union for GCREdit subtypes to preserve type safety.
207-243: Good: Telegram uses proper verification, but other contexts still vulnerableThe Telegram branch correctly uses
verifyWeb2Proof, but Twitter/GitHub fall back to the insecuresha256(proof) === proofHashcheck which is client-controlled.Unify all contexts to use
verifyWeb2Proof:let proofOk = false - - if (context === "telegram") { - // Telegram uses dual signature validation (user + bot signatures) - // The proof is a TelegramSignedAttestation object, not a URL - try { - // Import verifyWeb2Proof which handles telegram verification - const { verifyWeb2Proof } = await import("@/libs/abstraction") - - const verificationResult = await verifyWeb2Proof( - { - context: "telegram", - username: data.username, - userId: data.userId, - proof: data.proof, - }, - accountGCR.pubkey, // sender's ed25519 address - ) - - proofOk = verificationResult.success - - if (!proofOk) { - log.error( - `Telegram verification failed: ${verificationResult.message}`, - ) - return { - success: false, - message: verificationResult.message, - } - } - - log.info( - `Telegram identity verified: ${data.username} (${data.userId})`, - ) - } catch (error) { - log.error(`Telegram proof verification failed: ${error}`) - proofOk = false - } - } else { - // Standard SHA256 proof validation for other platforms - proofOk = Hashing.sha256(data.proof) === data.proofHash - } + try { + const { verifyWeb2Proof } = await import("@/libs/abstraction") + const verificationResult = await verifyWeb2Proof( + { + context, + username: data.username, + userId: data.userId, + proof: data.proof, + }, + accountGCR.pubkey, + ) + proofOk = verificationResult.success + if (!proofOk) { + log.error(`${context} verification failed: ${verificationResult.message}`) + return { + success: false, + message: verificationResult.message, + } + } + log.info(`${context} identity verified: ${data.username} (${data.userId})`) + } catch (error) { + log.error(`${context} proof verification failed: ${error}`) + return { + success: false, + message: `${context} verification failed`, + } + }src/libs/abstraction/index.ts (4)
8-8: Fix brittle import path from node_modules internalsImporting directly from
node_modules/.../build/typeswill break when packages are updated or in different package managers.Apply this fix:
-import type { GenesisBlock } from "node_modules/@kynesyslabs/demosdk/build/types/blockchain/blocks" // TODO Properly import from types +import type { GenesisBlock } from "@kynesyslabs/demosdk/blockchain"If the type isn't exported from the public API, request it from the SDK maintainers or define a local type until it's available.
128-128: JSON.stringify is not deterministic for signature verificationUsing
JSON.stringifyfor signature verification can fail intermittently due to non-deterministic key ordering across runtimes.Implement canonical JSON serialization:
- const messageToVerify = JSON.stringify(attestationPayload) + const messageToVerify = canonicalStringify(attestationPayload) + + function canonicalStringify(obj: unknown): string { + if (obj === null || typeof obj !== "object") return JSON.stringify(obj) + if (Array.isArray(obj)) return `[${obj.map(canonicalStringify).join(",")}]` + const entries = Object.entries(obj as Record<string, unknown>) + .sort(([a], [b]) => a.localeCompare(b)) + return `{${entries.map(([k, v]) => `${JSON.stringify(k)}:${canonicalStringify(v)}`).join(",")}}` + }
146-153: Missing replay attack protectionThe attestation lacks nonce/timestamp validation, allowing attestations to be replayed indefinitely.
Add timestamp validation and nonce tracking:
+ // Check attestation freshness (must be within 5 minutes) + if (attestationPayload.timestamp) { + const attestTime = new Date(attestationPayload.timestamp).getTime() + const now = Date.now() + if (Math.abs(now - attestTime) > 5 * 60 * 1000) { + return { + success: false, + message: "Attestation expired or clock skew detected" + } + } + } + // Check bot authorization - bot must have balance in genesis block const botAuthorized = await checkBotAuthorization(botAddress)Consider implementing a nonce store to prevent replay of valid attestations.
113-125: Critical: User signature verification uses wrong messageThe user signature is being verified against the challenge string only, not the full attestation payload. This allows replay attacks where the same challenge signature can be reused in different attestations.
The user should sign the entire attestation payload to prevent tampering:
- // INFO: Verify user signature + // INFO: Verify user signature over the full attestation payload + const userMessage = JSON.stringify({ + telegram_user_id: attestationPayload.telegram_user_id, + username: attestationPayload.username, + challenge: attestationPayload.challenge, + public_key: attestationPayload.public_key, + bot_address: attestationPayload.bot_address + }) const userSignatureValid = await ucrypto.verify({ algorithm: "ed25519", - message: new TextEncoder().encode(attestationPayload.challenge), + message: new TextEncoder().encode(userMessage), publicKey: hexToUint8Array(attestationPayload.public_key), signature: hexToUint8Array(attestationPayload.signature), })
| if (balance[1] >= MIN_BALANCE) { | ||
| identities.add(balance[0]) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
String comparison bug: balance check fails for numeric values
The balance comparison uses string comparison (balance[1] >= MIN_BALANCE), which produces incorrect results when balance[1] is a number. For example, 100 >= "1000000000000" returns false in JavaScript, excluding valid genesis identities.
Apply this fix to handle both numeric and string balance types:
- if (balance[1] >= MIN_BALANCE) {
+ const balanceValue = typeof balance[1] === 'string' ? balance[1] : String(balance[1])
+ const minBalanceValue = MIN_BALANCE
+ if (BigInt(balanceValue) >= BigInt(minBalanceValue)) {🏁 Script executed:
#!/bin/bash
# Verify the genesis.json structure to understand balance format
if [ -f data/genesis.json ]; then
head -20 data/genesis.json | jq '.balances[0:3]' 2>/dev/null || head -20 data/genesis.json
echo "---"
# Check if balances are numbers or strings
jq -r '.balances | map(.[1] | type) | unique | .[]' data/genesis.json 2>/dev/null || echo "Could not determine balance types"
fiLength of output: 564
Fix balance comparison to use BigInt
genesis.json balances are strings (e.g. "1000000000000000000"); if (balance[1] >= MIN_BALANCE) can do lexicographic/unsafe numeric comparison and exclude valid identities — compare as BigInt instead.
File: src/libs/blockchain/routines/loadGenesisIdentities.ts (around lines 11–13)
- if (balance[1] >= MIN_BALANCE) {
+ const balanceValue = typeof balance[1] === 'string' ? balance[1] : String(balance[1])
+ const minBalanceValue = MIN_BALANCE
+ if (BigInt(balanceValue) >= BigInt(minBalanceValue)) {📝 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.
| if (balance[1] >= MIN_BALANCE) { | |
| identities.add(balance[0]) | |
| } | |
| const balanceValue = typeof balance[1] === 'string' ? balance[1] : String(balance[1]) | |
| const minBalanceValue = MIN_BALANCE | |
| if (BigInt(balanceValue) >= BigInt(minBalanceValue)) { | |
| identities.add(balance[0]) | |
| } |
🤖 Prompt for AI Agents
In src/libs/blockchain/routines/loadGenesisIdentities.ts around lines 11 to 13,
the current comparison uses if (balance[1] >= MIN_BALANCE) which can perform
lexicographic/unsafe numeric comparison because balances in genesis.json are
strings; convert the balance string to a BigInt and compare against a BigInt
MIN_BALANCE (e.g. use BigInt(balance[1]) >= MIN_BALANCE or ensure MIN_BALANCE is
BigInt via BigInt(MIN_BALANCE)) so the numeric comparison is correct.
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
|
This PR modifies Files that will be reverted:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts (1)
205-246: CRITICAL: Insecure proof fallback bypasses verification for Twitter/GitHub/Discord.The sha256 fallback (lines 243-246) allows client-controlled
proofandproofHashvalues, bypassing identity verification for non-Telegram contexts. This was flagged in past reviews but remains unfixed.All contexts (twitter, github, discord, telegram) should use
verifyWeb2Proofconsistently, as shown insrc/libs/abstraction/index.ts(lines 172-258).Apply this diff to unify verification across all contexts:
- let proofOk = false - - if (context === "telegram") { - // Telegram uses dual signature validation (user + bot signatures) - // The proof is a TelegramSignedAttestation object, not a URL - try { - // Import verifyWeb2Proof which handles telegram verification - const { verifyWeb2Proof } = await import("@/libs/abstraction") - - const verificationResult = await verifyWeb2Proof( - { - context: "telegram", - username: data.username, - userId: data.userId, - proof: data.proof, - }, - accountGCR.pubkey, // sender's ed25519 address - ) - - proofOk = verificationResult.success - - if (!proofOk) { - log.error( - `Telegram verification failed: ${verificationResult.message}`, - ) - return { - success: false, - message: verificationResult.message, - } - } - - log.info( - `Telegram identity verified: ${data.username} (${data.userId})`, - ) - } catch (error) { - log.error(`Telegram proof verification failed: ${error}`) - proofOk = false - } - } else { - // Standard SHA256 proof validation for other platforms - proofOk = Hashing.sha256(data.proof) === data.proofHash - } + // Unified verification for all Web2 contexts using verifyWeb2Proof + let proofOk = false + try { + const { verifyWeb2Proof } = await import("@/libs/abstraction") + + const verificationResult = await verifyWeb2Proof( + { + context, // twitter, github, telegram, or discord + username: data.username, + userId: data.userId, + proof: data.proof, + }, + accountGCR.pubkey, + ) + + proofOk = verificationResult.success + + if (!proofOk) { + log.error( + `${context} verification failed: ${verificationResult.message}`, + ) + return { + success: false, + message: verificationResult.message, + } + } + + log.info( + `${context} identity verified: ${data.username} (${data.userId})`, + ) + } catch (error) { + log.error(`${context} proof verification failed: ${error}`) + proofOk = false + }Also update the error message on lines 248-258 to remove the Telegram-specific branch:
if (!proofOk) { return { success: false, - message: - context === "telegram" - ? "Telegram attestation validation failed" - : "Sha256 proof mismatch: Expected " + - data.proofHash + - " but got " + - Hashing.sha256(data.proof), + message: `${context} attestation validation failed`, } }
🧹 Nitpick comments (1)
src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts (1)
713-792: Remove commented-out code.This large block of dead code should be deleted entirely rather than commented out. Version control preserves the history if needed.
Apply this diff to remove the commented-out code:
- // if (type === "twitter") { - // /** - // * Check if this Twitter userId exists anywhere - // */ - // const result = await gcrMainRepository - // .createQueryBuilder("gcr") - // .where( - // "EXISTS (SELECT 1 FROM jsonb_array_elements(gcr.identities->'web2'->'twitter') as twitter_id WHERE twitter_id->>'userId' = :userId)", - // { - // userId: data.userId, - // }, - // ) - // .andWhere("gcr.pubkey != :currentAccount", { currentAccount }) - // .getOne() - - // /** - // * Return true if no account has this userId - // */ - // return !result - // } else if (type === "github") { - // /** - // * Check if this GitHub userId exists anywhere - // */ - // const result = await gcrMainRepository - // .createQueryBuilder("gcr") - // .where( - // "EXISTS (SELECT 1 FROM jsonb_array_elements(gcr.identities->'web2'->'github') as github_id WHERE github_id->>'userId' = :userId)", - // { - // userId: data.userId, - // }, - // ) - // .andWhere("gcr.pubkey != :currentAccount", { currentAccount }) - // .getOne() - - // /** - // * Return true if no account has this userId - // */ - // return !result - // } else if (type === "discord") { - // /** - // * Check if this Discord userId exists anywhere - // */ - // const result = await gcrMainRepository - // .createQueryBuilder("gcr") - // .where( - // "EXISTS (SELECT 1 FROM jsonb_array_elements(COALESCE(gcr.identities->'web2'->'discord', '[]'::jsonb)) AS discord_id WHERE discord_id->>'userId' = :userId)", - // { userId: data.userId }, - // ) - // .andWhere("gcr.pubkey != :currentAccount", { currentAccount }) - // .getOne() - - // /** - // * Return true if no account has this userId - // */ - // return !result - // } else { /** * For web3 wallets, check if this address exists in any account for this chain/subchain */ const addressToCheck = data.chain === "evm" ? data.address.toLowerCase() : data.address const result = await gcrMainRepository .createQueryBuilder("gcr") .where( "EXISTS (SELECT 1 FROM jsonb_array_elements(gcr.identities->'xm'->:chain->:subchain) as xm_id WHERE xm_id->>'address' = :address)", { chain: data.chain, subchain: data.subchain, address: addressToCheck, }, ) .andWhere("gcr.pubkey != :currentAccount", { currentAccount }) .getOne() /** * Return true if this is the first connection */ return !result - // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/features/incentive/PointSystem.ts(7 hunks)src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts(15 hunks)src/libs/blockchain/gcr/gcr_routines/IncentiveManager.ts(1 hunks)src/libs/identity/tools/twitter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libs/blockchain/gcr/gcr_routines/IncentiveManager.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/features/incentive/PointSystem.ts (1)
src/libs/blockchain/gcr/gcr_routines/ensureGCRForUser.ts (1)
ensureGCRForUser(8-33)
src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts (3)
src/libs/abstraction/index.ts (1)
verifyWeb2Proof(173-259)src/libs/crypto/hashing.ts (1)
Hashing(15-26)src/libs/blockchain/gcr/gcr_routines/IncentiveManager.ts (1)
IncentiveManager(9-135)
🔇 Additional comments (11)
src/libs/identity/tools/twitter.ts (1)
580-584: LGTM! Directory initialization is correctly placed.The directory creation logic is appropriately placed in the singleton initialization, uses recursive creation, and prevents write failures in methods like
getTimelineandgetFollowers.src/features/incentive/PointSystem.ts (7)
16-16: LGTM! Telegram point value is consistent with GitHub.The
LINK_TELEGRAM: 1value aligns with the existingLINK_GITHUB: 1point value, maintaining consistency across social platform linkages.
123-131: LGTM! Safe defaults prevent undefined access.Initializing
telegramwith nullish coalescing ensures safe default values and consistency with other social accounts (twitter, github, discord).
160-166: LGTM! Defensive initialization prevents structure issues.Ensuring the breakdown structure exists before assignment guards against missing keys and aligns with the REVIEW comment's intent.
548-550: LGTM! Negative-points bug is fixed.The safe default (
?? 0) prevents the undefined ≤ 0 bug flagged in past reviews, correctly short-circuiting deduction when no Twitter points exist.
606-656: LGTM! GitHub deduction mirrors the Twitter fix.The refactored logic uses a safe default for
currentGithub, preventing negative points and maintaining consistency with the Twitter deduction flow.
658-740: LGTM! Telegram award flow follows established patterns.Ownership verification, idempotency checks, and point awarding are consistent with
awardGithubPoints. The theoretical race condition noted in past reviews (two parallel requests passing the "already awarded" check before save) remains unmitigated but is acceptable for the current implementation.
755-756: LGTM! Telegram deduction uses safe default.Consistent with the Twitter and GitHub fixes, the safe default prevents undefined ≤ 0 evaluation and negative points.
src/libs/blockchain/gcr/gcr_routines/GCRIdentityRoutines.ts (3)
297-310: LGTM! Telegram incentive hook is correctly integrated.The Telegram linking flow mirrors the Twitter and GitHub patterns, with proper first-connection checks and incentive manager invocation.
355-390: LGTM! Telegram unlinking follows established patterns.Tracking
removedIdentityfor Telegram and invokingIncentiveManager.telegramUnlinkedis consistent with the GitHub unlinking flow.
699-711: LGTM! Template-based query is cleaner than per-context branches.The unified template approach using dynamic type insertion is more maintainable than the commented-out per-context branches and correctly addresses the past review's concern about Telegram support.
|
This PR modifies Files that will be reverted:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/libs/blockchain/gcr/gcr.ts (1)
643-674: Document scaffolding for phased rollout.The commented-out
getAccountByTelegramUsernamemethod appears to be scaffolding for a future phase of the Telegram integration. Add a TODO or FIXME comment explaining the rollout phase and timeline for when this will be enabled.Apply this diff to add documentation:
+// TODO: Phase 2/3 - Telegram Identity Integration +// This method will be enabled in a future phase once Telegram bot verification +// and dual-signature validation are fully deployed and tested. +// See: [link to implementation plan or phase documentation] // static async getAccountByTelegramUsername(username: string) {Alternatively, if this code is not planned for near-term use, consider removing it entirely and reintroducing it when needed to reduce maintenance burden.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/libs/blockchain/gcr/gcr.ts(3 hunks)src/libs/network/manageGCRRoutines.ts(1 hunks)src/utilities/sharedState.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/libs/network/manageGCRRoutines.ts
- src/utilities/sharedState.ts
🔇 Additional comments (1)
src/libs/blockchain/gcr/gcr.ts (1)
617-626: LGTM!The
eslint-disable-next-line prefer-constis correctly applied sincechainis reassigned when it equals"eth"(line 625). The disable is justified and appropriate.
| static async getAccountByTwitterUsername(username: string) { | ||
| const db = await Datasource.getInstance() | ||
| const gcrMainRepository = db.getDataSource().getRepository(GCRMain) | ||
|
|
||
| // INFO: Find all accounts that have the twitter identity with the given username using a jsonb query | ||
| const accounts = await gcrMainRepository | ||
| .createQueryBuilder("gcr") | ||
| .where( | ||
| "EXISTS (SELECT 1 FROM jsonb_array_elements(gcr.identities->'web2'->'twitter') as twitter_id WHERE twitter_id->>'username' = :username)", | ||
| { username }, | ||
| ) | ||
| .getMany() | ||
|
|
||
| // If no accounts found, return null | ||
| if (accounts.length === 0) { | ||
| return null | ||
| } | ||
|
|
||
| // If only one account found, return it | ||
| if (accounts.length === 1) { | ||
| return accounts[0] | ||
| } | ||
|
|
||
| // If multiple accounts found, find the one that was awarded points | ||
| // (Twitter points > 0 means the account was awarded points) | ||
| const accountWithPoints = accounts.find( | ||
| account => account.points?.breakdown?.socialAccounts?.twitter > 0, | ||
| ) | ||
|
|
||
| // Return the account with points if found, otherwise return the first account | ||
| return accountWithPoints || accounts[0] | ||
| } |
There was a problem hiding this comment.
Clarify tie-breaking logic and add error handling.
The method has two concerns:
-
Tie-breaking ambiguity: If multiple accounts have
twitter > 0,.find()returns the first match without guaranteed ordering, leading to non-deterministic results. Consider using.sort()before.find()to establish a consistent tie-breaking rule (e.g., by registration date, total points, or explicit priority). -
Missing error handling: Database errors will propagate unhandled. Wrap the query in a try-catch block to handle potential database or query failures gracefully.
Apply this diff to add error handling:
static async getAccountByTwitterUsername(username: string) {
+ try {
const db = await Datasource.getInstance()
const gcrMainRepository = db.getDataSource().getRepository(GCRMain)
// INFO: Find all accounts that have the twitter identity with the given username using a jsonb query
const accounts = await gcrMainRepository
.createQueryBuilder("gcr")
.where(
"EXISTS (SELECT 1 FROM jsonb_array_elements(gcr.identities->'web2'->'twitter') as twitter_id WHERE twitter_id->>'username' = :username)",
{ username },
)
.getMany()
// If no accounts found, return null
if (accounts.length === 0) {
return null
}
// If only one account found, return it
if (accounts.length === 1) {
return accounts[0]
}
// If multiple accounts found, find the one that was awarded points
// (Twitter points > 0 means the account was awarded points)
const accountWithPoints = accounts.find(
account => account.points?.breakdown?.socialAccounts?.twitter > 0,
)
// Return the account with points if found, otherwise return the first account
return accountWithPoints || accounts[0]
+ } catch (error) {
+ log.error("Error fetching account by Twitter username:", error)
+ return null
+ }
}For the tie-breaking logic, consider sorting accounts before applying .find():
// If multiple accounts found, find the one that was awarded points
// (Twitter points > 0 means the account was awarded points)
- const accountWithPoints = accounts.find(
+ const accountWithPoints = accounts
+ .sort((a, b) =>
+ (b.points?.breakdown?.socialAccounts?.twitter || 0) -
+ (a.points?.breakdown?.socialAccounts?.twitter || 0)
+ )
+ .find(
account => account.points?.breakdown?.socialAccounts?.twitter > 0,
)🤖 Prompt for AI Agents
In src/libs/blockchain/gcr/gcr.ts around lines 539 to 570, the method lacks
error handling and uses a non-deterministic tie-break when multiple accounts
match; wrap the DB query and subsequent logic in a try-catch that logs the error
and returns null (or rethrows if preferred), and replace the current .find()
tie-break with a deterministic sort (for example, sort by total points
descending then by registration/createdAt ascending or another explicit
priority) before selecting the preferred account so results are reproducible.
User description
I'll let CodeRabbit review and comment this but in short:
📁 Key Files Modified
🎯 Implementation Highlights
PR Type
Enhancement
Description
Complete Telegram identity integration with bot verification system
Telegram bot verification replacing manual verification approach
GCR identity linking with dual signature validation
Points system integration for Telegram account linking
Diagram Walkthrough
File Walkthrough
7 files
Add Telegram points system integrationImplement Telegram dual signature verificationAdd Telegram username account lookupIntegrate Telegram identity processingAdd Telegram linking incentive methodsAdd Telegram username RPC endpointAdd Telegram to social accounts structure2 files
Minor formatting and structure updatesFix quote consistency formatting7 files
Add aptos examples to ignore listAllow UPPER_CASE variable naming conventionAdd Claude memory preservation workflowAdd Claude PR warning workflowAdd Serena memory preservation workflowAdd Serena merge warning workflowAdd Serena project configuration3 files
Add consolidated project context documentationAdd complete Telegram system documentationUpdate implementation plan documentation1 files
Bump DemosSDK to version 2.4.7Summary by CodeRabbit
New Features
Bug Fixes
Dependency Updates
Chores
Documentation