-
Notifications
You must be signed in to change notification settings - Fork 29
fix: fix broken gh actions #1225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/scripts/auxLists/utils.ts (1)
73-78: Core fix looks good, but consider using the existing logger.The fix correctly addresses the "TypeError: terminated" issue by awaiting
response.json()before returning. However, the debug logging usesconsole.loginstead of the winston logger already imported and configured in this file.Consider refactoring to use winston for consistency and better log level control:
- console.log(`Fetched from ${url}:`, response.status) - const data = await response.json() - - console.log(`Data from ${url}:`, data) - - return data + const logger = getLogger('fetchWithApiKey') + logger.info(`Fetched from ${url}`, { status: response.status }) + + const data = await response.json() + logger.debug(`Data from ${url}`, { data }) + + return dataThis approach:
- Uses structured logging with winston
- Allows log level control (info/debug)
- Maintains consistency with the rest of the codebase
- Keeps logs out of production unless explicitly enabled
src/public/PermitInfo.8453.json (2)
1152-1156: Normalize naming style for consistency.Consider “Machines Cash” or project’s canonical casing instead of “machines-cash” to match list conventions.
If the hyphenated lowercase is the official token name, keep as-is.
4217-4219: Name casing alignment with Uniswap list.Here it’s “Redstone”; in Uniswap it’s “RedStone”. Recommend standardizing to one casing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/public/PermitInfo.1.json(3 hunks)src/public/PermitInfo.137.json(1 hunks)src/public/PermitInfo.42161.json(4 hunks)src/public/PermitInfo.43114.json(2 hunks)src/public/PermitInfo.56.json(14 hunks)src/public/PermitInfo.8453.json(13 hunks)src/public/Uniswap.8453.json(2 hunks)src/scripts/auxLists/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/scripts/auxLists/utils.ts (2)
src/scripts/cowFi-tokens.js (1)
url(129-129)src/scripts/downloadImages.js (1)
response(20-20)
🔇 Additional comments (24)
src/public/PermitInfo.137.json (1)
734-737: LGTM!The new RetaStake token entry is properly formatted, correctly positioned alphabetically, and consistent with the existing structure.
src/public/PermitInfo.8453.json (11)
32-36: Entry added: Klima Protocol kVCM — looks good.Structure and fields match existing entries.
1292-1296: Entry added: PLAY AI — looks good.No structural issues spotted.
1522-1526: Entry added: Lisk — cross-file consistency check.Permit info present here; Uniswap token also added. Please confirm this contract actually supports EIP‑2612 and that the “version” is correct for this deployment.
2017-2021: Entry added: SURGE — looks good.No structural issues spotted.
2168-2171: Entry added: Folks Finance — looks good.No structural issues spotted.
3037-3039: Entry added: Small Thing (unsupported) — ok.No structural issues spotted.
3585-3587: Entry added: Tonomy Token (unsupported) — ok.No structural issues spotted.
4941-4943: Entry added: Intuition (unsupported) — ok.No structural issues spotted.
5305-5307: Entry added: Analog (unsupported) — ok.No structural issues spotted.
7789-7791: Entry added: Titan (unsupported) — ok.No structural issues spotted.
1882-1886: Confirm Warplet addresses are distinct projects and verify downstream tooling behavior.The duplicate name "Warplet" is confirmed at two addresses:
0x3d81fbe679e8aa27a290b15aab5af5fa23cdcca3and0xd9159ad2d5fe625cd1f54f4d328fb19cb5262b07. Additionally, this appears to be a systemic pattern throughout the file—30+ other names also map to multiple distinct addresses.Unable to locate usage of this file in the codebase or documentation about expected handling of duplicates. Please verify: (1) these are intentionally different projects, (2) how downstream tooling consumes this file, and (3) whether name-based keying could cause collisions.
src/public/Uniswap.8453.json (2)
7-7: Version bump to 0.50 — correct for token additions.Minor version bump aligns with Token List spec for additions.
1596-1596: Timestamp updated — good.RFC 3339 format and aligns with version bump.
src/public/PermitInfo.43114.json (2)
777-781: Entry addition is well-formed and properly positioned.The "Folks Finance" entry at address 0xff7f8f301f7a706e3cfd3d2275f5dc0b9ee8009b follows the established JSON structure and maintains alphabetical ordering within the file.
1634-1637: Entry addition is well-formed and properly positioned.The "Kite" entry at address 0x904567252d8f48555b7447c67dca23f0372e16be follows the established JSON structure (unsupported type, no version field) and maintains alphabetical ordering.
src/public/PermitInfo.42161.json (4)
1712-1716: Entry addition is well-formed and properly positioned.The "Hathor Token" entry at address 0x87ca1ac7697c1240518b464b02e92a856d81aee1 follows the established JSON structure with eip-2612 type and version "1", maintaining alphabetical ordering.
3067-3071: Cross-file consistency confirmed for Folks Finance entry.The "Folks Finance" entry (0xff7f8f301f7a706e3cfd3d2275f5dc0b9ee8009b) is consistent with the same entry in PermitInfo.43114.json, suggesting a deliberate multi-chain token catalog expansion.
3894-3897: Additional entries are well-formed and properly positioned.The "Ostrich" (0x475e1cc57af3e40c80570220b1ce079b219605c4) and "Capx" (0x97e66d3c4d5bcd7c64e3e55af28544c9addf9281) entries both follow the unsupported token structure and maintain correct alphabetical ordering within the file.
Also applies to: 4714-4717
1-100: Note: Utility function code not provided for review.The PR objectives describe fixing broken GitHub Actions by "awaiting the JSON promise before returning the response" and mention "a debug log that consumes response.json() first." However, the provided files are only JSON data files—the actual source code file containing the utility function enhancement is not included in this review. To fully validate the PR objectives, the modified source file (likely JavaScript/TypeScript) should be reviewed.
src/public/PermitInfo.56.json (1)
1-4229:⚠️ Misalignment between PR objectives and file content.This pull request is described as fixing broken GitHub Actions with logging improvements, but the file being reviewed is purely static JSON data containing token permit mappings. This file does not appear to contain the utility function changes mentioned in the PR description.
The JSON data itself is valid—all addresses are properly formatted, entries are well-formed, and the file structure is sound. However, to verify the scope and intent of this PR, please clarify:
- Does this data file addition relate to the GitHub Actions/logging fixes, or is it a separate bundled change?
- Are there accompanying code files (utility functions with logging changes) that were modified to fix the broken GitHub Actions issue mentioned in the PR objectives?
src/public/PermitInfo.1.json (3)
6697-6700: Kite consistency verified across chainsThe address 0x904567252d8f48555b7447c67dca23f0372e16be is consistently marked as "unsupported" with name "Kite" across all chain files where it appears (mainnet, Avalanche, Binance Testnet). No inconsistencies detected.
2282-2286: I need to verify the contract details for the Folks Finance token on Ethereum.Good! The contract address 0xff7f8f301f7a706e3cfd3d2275f5dc0b9ee8009b is confirmed as the FOLKS token contract on Ethereum. Now I need to verify EIP-2612 v1 support.
I found general EIP-2612 information, but I need to specifically verify if FOLKS token has permit v1 support. Let me search for more specific contract details.
Let me check the actual JSON content to better understand what data is already recorded:
Based on my verification:
Verified:
- The contract address 0xff7f8f301f7a706e3cfd3d2275f5dc0b9ee8009b is confirmed as the official FOLKS token on Ethereum
- The JSON is valid and the address appears exactly once in PermitInfo.1.json (line 2282)
- The token is not a bridge token—it's the canonical FOLKS token on multiple EVM chains
Unable to independently verify:
I could not find definitive proof of EIP-2612 v1 permit support from public sources. The fact that this entry is already in PermitInfo.1.json with type "eip-2612" and version "1" suggests prior verification occurred during PR review. To definitively confirm permit implementation, you would need to inspect the contract on Etherscan or review the contract's ABI/source code directly.Confirm mainnet address and permit v1 support by:
- Checking the contract on Etherscan to verify the
permitfunction exists in the contract ABI- Verifying the function signature matches the EIP-2612 standard
712-716: Lit Protocol address and EIP-2612 support verifiedThe LitToken contract at 0x4d4eb0e8b160f6ebf63cc6d36060ffec09301b42 is the official Lit Protocol token on Ethereum mainnet and implements ERC20Permit, confirming EIP-2612 v1 support. JSON structure is valid, key is unique, no cross-file conflicts detected, and trailing commas are properly formatted.
78412d4 to
16e790b
Compare
|
And now they are succeeding again without any changes?? https://github.com/cowprotocol/token-lists/actions/runs/19153663495/job/54749558653 Honestly, still don't know what caused it or how it was fixed. |
Summary
Fix broken GH actions by awaiting the JSON promise before returning.
The error msg that was being thrown was:
Error fetching Coingecko's coin list: TypeError: terminatedLooking at the code, it doesn't seem to indicate the request failed.
So I added a debug log that first consumes the response.json() to log it, then returns it.
That solved it.
Both entries were logged and the response returned as expected.
Summary by CodeRabbit