feat(synapse-constants): Refactor with rollup for package build and export [SLT-160] #3175
Conversation
WalkthroughThe changes involve the introduction of a new ESLint configuration file, updates to the TypeScript configuration, and the addition of a Rollup configuration for building the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
javascript
Deploying sanguine-fe with
|
| Latest commit: |
f132586
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ca3c8a78.sanguine-fe.pages.dev |
| Branch Preview URL: | https://synapse-constants-refactor.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3175 +/- ##
=============================================
Coverage 90.56974% 90.56974%
=============================================
Files 54 54
Lines 1018 1018
Branches 82 82
=============================================
Hits 922 922
Misses 93 93
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
packages/synapse-constants/src/index.ts (1)
4-6: Consider removing commented-out exportsThe commented-out export statements for assets (chains, explorers, and icons) may lead to confusion and clutter in the codebase. It's generally better to remove unused code.
If these exports are planned for future implementation, consider replacing them with TODO comments. For example:
// TODO: Export assets when they are ready // TODO: export * from './constants/assets/chains/index' // TODO: export * from './constants/assets/explorer/index' // TODO: export * from './constants/assets/icons/index'This approach clearly indicates future plans without leaving unused code in the file.
packages/synapse-constants/src/scripts/generateMaps.cjs (1)
Line range hint
1-438: Suggestions for future improvementsWhile the current changes look good, here are some suggestions for future improvements to this file:
- Consider splitting this large file into smaller, more focused modules to improve maintainability.
- Modernize the codebase by transitioning from CommonJS to ES modules, which would align better with the async/await syntax already in use.
- Enhance error handling, particularly around API calls and contract interactions, to improve robustness.
- Refactor long functions like
printMapsto improve readability and maintainability.These suggestions are not critical for the current PR but could be considered for future refactoring efforts.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (94)
packages/synapse-constants/src/constants/assets/chains/arbitrum.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/aurora.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/avalanche.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/base.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/blast.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/bnb.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/boba.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/canto.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/cronos.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/dfk.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/dogechain.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/ethereum.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/fantom.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/harmony.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/klaytn.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/linea.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/metis.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/moonbeam.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/moonriver.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/optimism.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/polygon.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/chains/scroll.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/arbitrum.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/aurora.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/avalanche.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/basescan.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/blast.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/boba.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/bscscan.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/canto.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/cronos.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/dfk-chain.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/dogechain.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/etherscan.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/fantom.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/harmony.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/klaytn.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/linea.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/metis.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/moonbeam.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/moonriver.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/optimism.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/polygon.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/explorer/scroll.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/ageur.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/avax.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/avweth.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/btc.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/busd.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/crvusd.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/dai.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/dog.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/eth.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/frax.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/ftm.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/fusdt.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/gmx.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/gohm.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/h2o.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/highstreet.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/hyperjump.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/jewel.pngis excluded by!**/*.pngpackages/synapse-constants/src/constants/assets/icons/klay.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/l2dao.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/link.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/matic.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/metamask.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/mim.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/movr.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/neth.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/newo.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/nfd.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/note.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/nusd.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/ohm.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/op.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/pepe.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/pls.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/sdt.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/sfi.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/solar.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/spectral.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/susd.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/sushi.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/syn.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/unidex.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/usdb.pngis excluded by!**/*.pngpackages/synapse-constants/src/constants/assets/icons/usdb.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/usdc.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/usdt.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/vsta.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/wbtc.svgis excluded by!**/*.svgpackages/synapse-constants/src/constants/assets/icons/weth.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (12)
- packages/synapse-constants/.eslintrc.cjs (1 hunks)
- packages/synapse-constants/index.ts (0 hunks)
- packages/synapse-constants/package.json (2 hunks)
- packages/synapse-constants/rollup.config.js (1 hunks)
- packages/synapse-constants/src/constants/tokens/index.ts (1 hunks)
- packages/synapse-constants/src/constants/tokens/poolMaster.ts (0 hunks)
- packages/synapse-constants/src/constants/tokens/sushiMaster.ts (0 hunks)
- packages/synapse-constants/src/constants/types/index.ts (1 hunks)
- packages/synapse-constants/src/index.ts (1 hunks)
- packages/synapse-constants/src/scripts/generateMaps.cjs (1 hunks)
- packages/synapse-constants/tsconfig.json (1 hunks)
- packages/synapse-constants/webpack.config.js (0 hunks)
Files not reviewed due to no reviewable changes (4)
- packages/synapse-constants/index.ts
- packages/synapse-constants/src/constants/tokens/poolMaster.ts
- packages/synapse-constants/src/constants/tokens/sushiMaster.ts
- packages/synapse-constants/webpack.config.js
Additional comments not posted (23)
packages/synapse-constants/.eslintrc.cjs (3)
1-12: LGTM: ESLint configuration structure looks good.The ESLint configuration is well-structured, extending from the parent configuration and providing specific overrides for TypeScript files. This approach allows for customized linting rules in the
synapse-constantspackage while maintaining consistency with the overall project standards.
8-8: Clarify the decision to disable 'prefer-arrow/prefer-arrow-functions'.Disabling the 'prefer-arrow/prefer-arrow-functions' rule allows more flexibility in function declarations, but it may lead to inconsistencies in coding style across the project.
Could you explain the reasoning behind disabling this rule? If it's a deliberate choice, consider documenting it in the configuration file or project documentation.
#!/bin/bash # Search for function declarations in TypeScript files that might be affected by disabling 'prefer-arrow/prefer-arrow-functions' rg --type typescript "function\s+\w+\s*\(" packages/synapse-constants
7-7: Consider the implications of disabling 'guard-for-in' rule.The 'guard-for-in' rule is typically used to ensure that
for...inloops checkhasOwnPropertyto avoid iterating over inherited properties. Disabling this rule might lead to unexpected behavior if not handled carefully in the code.Could you provide more context on why this rule needs to be disabled? If it's necessary, consider adding a comment in the configuration file explaining the rationale.
Verification successful
Disabling the 'guard-for-in' rule is safe as no
for...inloops were found in thepackages/synapse-constantsdirectory.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for 'for...in' loops in TypeScript files that might be affected by disabling 'guard-for-in' rg --type typescript "for\s*\(\s*\w+\s+in\s+" packages/synapse-constantsLength of output: 106
Script:
#!/bin/bash # Search for 'for...in' loops in TypeScript files within packages/synapse-constants rg --type ts "for\s*\(\s*\w+\s+in\s+\w+\s*\)" packages/synapse-constantsLength of output: 74
Script:
#!/bin/bash # Search for 'for...in' loops in TypeScript files within packages/synapse-constants using ast-grep ast-grep --lang typescript --pattern 'for ($_ in $_) { $$$ }' packages/synapse-constantsLength of output: 90
packages/synapse-constants/src/index.ts (3)
1-1: LGTM: Proper export of token constantsThis export statement correctly re-exports all token-related constants, making them easily accessible to users of the package.
2-2: Well-structured export for chain constantsExporting chain-related constants under the CHAINS namespace is a good practice. It provides clear organization and helps prevent potential naming conflicts.
3-3: LGTM: Proper export of type constantsThis export statement correctly re-exports all type-related constants, providing easy access to users of the package.
packages/synapse-constants/tsconfig.json (5)
6-6: Approve enabling declaration file generationEnabling the generation of declaration files is a positive change. It will provide better type support for consumers of this package.
7-7: Approve outDir path updateThe change to use "dist" without the leading "./" for the outDir is a good standardization. It aligns with common conventions and improves consistency.
12-13: Approve updates to include and exclude patternsThe changes to the include and exclude patterns are good improvements:
- Simplifying the include pattern to
"src/**/*"makes the configuration more maintainable and automatically includes new files.- Adding "dist" to the exclude list prevents potential issues with the compiler processing output files.
These changes will help keep the compilation process clean and efficient.
8-8: Approve enabling strict modeEnabling strict mode is a great step towards improving code quality. It will help catch more errors at compile-time and reduce potential runtime issues.
Please ensure that the existing codebase has been thoroughly tested with this change. Run the following script to check for any new TypeScript errors:
#!/bin/bash # Description: Check for TypeScript errors after enabling strict mode # Test: Run TypeScript compiler in noEmit mode to check for errors npx tsc --noEmit
3-3: Approve module change to ESNextThe change to use "ESNext" for the module option is a good modernization step. It allows the use of the latest ECMAScript module syntax.
To ensure compatibility, please run the following script to check for any potential issues with module imports/exports:
packages/synapse-constants/rollup.config.js (2)
1-7: LGTM: Import statements are well-organized and comprehensive.The import statements cover all necessary Rollup plugins for the build process, including TypeScript support, module resolution, CommonJS conversion, JSON handling, minification, URL handling, and code coverage reporting. The imports are clear and concise, with no unused imports present.
9-22: LGTM: Input and output configuration is well-defined.The configuration correctly specifies:
- The entry point as 'src/index.ts'.
- Two output formats: CommonJS (cjs) and ES Module (esm).
- Appropriate output file names and locations in the 'dist' directory.
- Sourcemaps enabled for both output formats, which is beneficial for debugging.
This setup ensures compatibility with different module systems and provides developer-friendly debugging capabilities.
packages/synapse-constants/package.json (7)
19-19: Good improvement in linting configuration.Specifying the ESLint configuration file explicitly (.eslintrc.cjs) ensures consistent linting across different environments. This is a good practice, especially when using ES modules.
35-40: LGTM! Rollup dependencies added.The addition of Rollup and its plugins as devDependencies aligns with the PR objective of refactoring the build process. These replace the previously used Webpack-related dependencies and should provide a more efficient and flexible build setup.
42-43: Good addition of Rollup and TypeScript plugin.The inclusion of rollup and rollup-plugin-typescript2 as devDependencies completes the setup for using Rollup with TypeScript. This should provide an efficient build process for the TypeScript codebase.
29-31: New dependencies added. Verify their usage.The addition of @ethersproject libraries and viem is likely to support new features or improve existing functionality. The inclusion of viem, a modern Ethereum library, could lead to more efficient Ethereum interactions.
Run the following script to check the usage of these new dependencies in the codebase:
#!/bin/bash # Check usage of new dependencies rg --type typescript '@ethersproject/bignumber|@ethersproject/address|viem' packages/synapse-constants/src
45-45: TypeScript updated to v5. Ensure compatibility.Upgrading to TypeScript 5.3.3 is a good practice for accessing the latest features and bug fixes. However, as this is a major version upgrade, it's crucial to thoroughly test the codebase to ensure there are no breaking changes or unexpected behaviors.
Run the following script to check for any TypeScript errors after the upgrade:
#!/bin/bash # Check for TypeScript errors after upgrade cd packages/synapse-constants yarn tsc --noEmit
20-23: LGTM! Verify the new build process.The updated scripts reflect the migration to Rollup and improve the build process. The use of
--bundleConfigAsCjsensures compatibility with CommonJS config files, which is a good practice.Please run the following script to verify the new build process:
5-7: LGTM! Verify new bundle paths.The changes to module type and entry points align well with the migration to Rollup and ES modules. This should improve package compatibility and build efficiency.
Please run the following script to ensure the new bundle files are correctly generated:
packages/synapse-constants/src/constants/types/index.ts (3)
4-4: LGTM: Import statement restoredThe import of CHAINS from '../chains/master' has been correctly restored. This change resolves a potential reference error in the
makeMultiChainObjfunction, which usesCHAINSto iterate over chain IDs.
Line range hint
1-279: Summary: Minor import change with positive impactThe only change in this file is the restoration of the CHAINS import, which is crucial for the
makeMultiChainObjfunction. This change aligns with the PR objectives of refactoring and improving the project structure. The rest of the file, including extensive type definitions and the Token class, remains unchanged and appears to be well-structured.
4-4: Verify the imported modulePlease ensure that the '../chains/master' module exists and exports the expected CHAINS object. This import is crucial for the
makeMultiChainObjfunction to work correctly.Run the following script to verify the module:
| plugins: [ | ||
| nodeResolve({ | ||
| preferBuiltins: true, | ||
| }), | ||
| commonjs(), | ||
| json(), | ||
| typescript({ | ||
| tsconfig: './tsconfig.json', | ||
| declaration: true, | ||
| declarationDir: 'dist', | ||
| }), | ||
| terser(), | ||
| url({ | ||
| include: ['**/*.svg', '**/*.png', '**/*.jpg'], | ||
| limit: 0, | ||
| emitFiles: false, | ||
| }), | ||
| codecovRollupPlugin({ | ||
| enableBundleAnalysis: process.env.CODECOV_TOKEN !== undefined, | ||
| bundleName: 'synapse-constants', | ||
| uploadToken: process.env.CODECOV_TOKEN, | ||
| uploadOverrides: { | ||
| sha: process.env.GH_COMMIT_SHA, | ||
| }, | ||
| }), | ||
| ], |
There was a problem hiding this comment.
LGTM: Plugin configuration is comprehensive and well-structured.
The plugin setup covers all necessary aspects of the build process:
- Module resolution and CommonJS conversion.
- JSON and TypeScript support.
- Minification with terser.
- Handling of image files.
- Code coverage reporting with Codecov.
The TypeScript plugin is correctly configured with the appropriate tsconfig and declaration settings, which is crucial for maintaining type information.
Consider extracting the Codecov configuration into a separate object for improved readability:
const codecovConfig = {
enableBundleAnalysis: process.env.CODECOV_TOKEN !== undefined,
bundleName: 'synapse-constants',
uploadToken: process.env.CODECOV_TOKEN,
uploadOverrides: {
sha: process.env.GH_COMMIT_SHA,
},
};
// In the plugins array:
codecovRollupPlugin(codecovConfig),This change would make the configuration more modular and easier to maintain.
| @@ -51,7 +51,7 @@ export const findChainIdsWithPausedToken = (routeSymbol: string) => { | |||
| PAUSED_TOKENS_BY_CHAIN, | |||
| (result, tokens, chainId) => { | |||
| if (_.includes(tokens, routeSymbol)) { | |||
| result.push(chainId) | |||
| result.push(chainId as never) | |||
There was a problem hiding this comment.
Reconsider the use of as never type assertion
The change from result.push(chainId) to result.push(chainId as never) introduces a potentially problematic type assertion. The never type in TypeScript represents values that never occur, which doesn't align with the expected string type of chainId.
This change might be attempting to bypass a TypeScript compilation error, but it's not addressing the root cause of any potential type mismatch. Instead, consider the following alternatives:
-
Explicitly type the
resultarray:const result: string[] = [];
-
Use a type guard to ensure
chainIdis a string:if (typeof chainId === 'string') { result.push(chainId); }
-
If there's a specific reason for using
as never, please add a comment explaining the rationale to improve code maintainability.
Bundle ReportChanges will increase total bundle size by 320.5kB (0.9%) ⬆️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
packages/synapse-constants/src/utils/makeMultiChainObj.ts (1)
7-10: Efficient object population with room for optimization.The code effectively creates and populates an object with chain IDs as keys. This approach is straightforward and works well.
For potentially better performance, especially with a large number of chains, consider using
Object.fromEntriesandmap:const obj = Object.fromEntries( Object.values(CHAINS).map(chain => [chain.id, valOrObj]) );This approach creates the object in a single operation, which might be more efficient.
packages/synapse-constants/src/utils/validateAddresses.ts (1)
3-5: Consider extracting the repeated type to a type alias.The type
{ [x: number]: string }is used multiple times in the function signature. For better readability and maintainability, consider extracting it to a type alias.Here's a suggested implementation:
type AddressMap = { [chainId: number]: string } export const validateAddresses = (addresses: AddressMap): AddressMap => { // ... rest of the function implementation }This change improves code readability and makes it easier to update the type definition if needed in the future.
packages/synapse-constants/package.json (3)
24-28: Great updates to build and lint scripts!The changes to the scripts section effectively implement the transition to Rollup and improve the build process. The specific ESLint config file ensures consistent linting across the project.
Consider adding a
lint:fixscript that runs ESLint with the--fixflag to automatically fix some linting issues:"lint:fix": "eslint . --fix --config .eslintrc.cjs"
31-35: Good updates to dependencies!The addition of specific ethersproject packages and viem suggests a more focused and potentially more efficient use of Ethereum-related libraries. The inclusion of lodash is also beneficial for utility functions.
Consider pinning the versions of critical dependencies like viem to ensure consistent builds:
"viem": "2.21.6"This helps prevent unexpected breaking changes in future builds.
38-46: Excellent transition to Rollup and TypeScript update!The changes in devDependencies effectively implement the transition from Webpack to Rollup, aligning perfectly with the PR objectives. The variety of Rollup plugins added suggests a well-thought-out build process. The update to TypeScript 5.3.3 is also a great improvement, bringing in the latest features and performance enhancements.
To improve maintainability, consider adding a comment in the
rollup.config.jsfile explaining the purpose of each plugin and any non-obvious configuration choices. This will help future contributors understand the build process more easily.packages/synapse-constants/src/types/index.ts (1)
Line range hint
201-258: LGTM: Token class constructor refactored with utility functions.The Token class constructor has been improved:
validateAddressesis now used for theaddressesproperty, enhancing input validation.makeMultiChainObjis applied todecimalsandpoolId, likely standardizing the structure of multi-chain objects.- Default values have been adjusted, which seems intentional and aligned with the refactoring goals.
These changes improve code quality and consistency.
Consider adding a comment explaining the purpose of
makeMultiChainObjfor better code documentation:- this.decimals = makeMultiChainObj(decimals) + // Ensure decimals is a consistent multi-chain object + this.decimals = makeMultiChainObj(decimals)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
- packages/synapse-constants/package.json (1 hunks)
- packages/synapse-constants/rollup.config.js (1 hunks)
- packages/synapse-constants/src/constants/chains/index.ts (1 hunks)
- packages/synapse-constants/src/constants/chains/master.ts (1 hunks)
- packages/synapse-constants/src/constants/index.ts (1 hunks)
- packages/synapse-constants/src/constants/tokens/auxilliary.ts (1 hunks)
- packages/synapse-constants/src/constants/tokens/bridgeable.ts (1 hunks)
- packages/synapse-constants/src/constants/tokens/deprecated.ts (1 hunks)
- packages/synapse-constants/src/constants/tokens/index.ts (2 hunks)
- packages/synapse-constants/src/constants/tokens/poolMaster.ts (1 hunks)
- packages/synapse-constants/src/constants/tokens/sushiMaster.ts (1 hunks)
- packages/synapse-constants/src/constants/tokens/swapMaster.ts (1 hunks)
- packages/synapse-constants/src/index.ts (1 hunks)
- packages/synapse-constants/src/types/index.ts (4 hunks)
- packages/synapse-constants/src/utils/makeMultiChainObj.ts (1 hunks)
- packages/synapse-constants/src/utils/validateAddresses.ts (1 hunks)
- packages/synapse-constants/tsconfig.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/synapse-constants/rollup.config.js
- packages/synapse-constants/src/constants/tokens/index.ts
- packages/synapse-constants/src/constants/tokens/poolMaster.ts
- packages/synapse-constants/src/constants/tokens/sushiMaster.ts
- packages/synapse-constants/src/index.ts
- packages/synapse-constants/tsconfig.json
🔇 Additional comments (15)
packages/synapse-constants/src/constants/index.ts (1)
3-3: Verify the new import path and its impact on the codebase.The change in the export path from './types' to '../types' aligns with the PR objective of refactoring the folder and file organization. However, this modification may have broader implications:
- Ensure that the new path '../types' is correct and the types file exists in the parent directory.
- This change could potentially break existing imports in other parts of the codebase.
To verify the correctness of the new import path and its impact, please run the following script:
Please review the script output to ensure that:
- The types file exists in the new location.
- There are no remaining imports from the old location that need to be updated.
packages/synapse-constants/src/utils/makeMultiChainObj.ts (1)
4-6: Good use of type checking and early return.The function efficiently handles the case where
valOrObjis already an object, returning it immediately. This is a good practice that reduces unnecessary processing and improves code readability.packages/synapse-constants/src/utils/validateAddresses.ts (1)
1-13: LGTM! The implementation looks solid.The
validateAddressesfunction correctly reformats and validates Ethereum addresses using thegetAddressfunction from@ethersproject/address. The TypeScript types are well-defined, and the logic is clear and concise.packages/synapse-constants/package.json (4)
5-7: Excellent update to package entry points!The changes to the
main,module, andtypesfields are well-structured and align with modern best practices for package distribution. Separating CommonJS and ES modules allows for better compatibility across different environments, while the dedicated types directory improves organization.
8-8: Great addition of the license field!Adding the MIT license is an excellent choice for open-source projects. It clearly communicates the terms under which the package can be used and distributed.
12-16: Excellent addition of repository and homepage information!The inclusion of the repository and homepage fields greatly enhances the package's metadata. This makes it easier for users to locate the source code and documentation, improving the overall developer experience.
48-48: Good update to TypeScript!Updating to TypeScript 5.3.3 is a positive change, bringing in the latest features, performance improvements, and bug fixes.
To ensure we're taking full advantage of the new TypeScript version, please review the
tsconfig.jsonfile for any new compiler options that could be beneficial. You can run the following command to check for any new strict checks or features that could be enabled:packages/synapse-constants/src/constants/tokens/auxilliary.ts (2)
Line range hint
9-138: LGTM. No changes required in token definitions.The rest of the file, including all token definitions, remains unchanged and consistent with the updated import of the
Tokentype. The refactoring of the import path doesn't affect the functionality or structure of the token definitions.
7-7: LGTM. Verify import path consistency across the project.The change in the import path for the
Tokentype is in line with the project's refactoring objectives. This modification updates the relative path from '../types' to '../../types', which suggests a change in the directory structure.To ensure consistency across the project, please run the following script to verify the usage of the new import path:
✅ Verification successful
Action Required: Update and Run Verification Script for Import Paths
The previous search command failed because
typescriptis not a recognized file type. Please run the following updated script to verify that allTokenimports use the correct path'../../types':
Action Required: Correct the Regex and Run Verification Script Again
The previous script encountered a regex parse error due to unescaped curly braces. Please run the following updated script to verify that all
Tokenimports use the correct path'../../types':
Verified: Import Path Consistency Confirmed
All instances of
import { Token } from '../../types'across the project have been confirmed to use the updated path. The change inauxilliary.tsaligns with the project's refactoring objectives and maintains consistency throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of Token import paths across the project # Test: Search for Token imports from types. Expect: All imports should use '../../types' rg --type typescript "import.*Token.*from '.*types'" packages/synapse-constants/srcLength of output: 126
Script:
#!/bin/bash # Description: Verify the consistency of Token import paths across the project # Test: Search for Token imports from types. Expect: All imports should use '../../types' rg "import\s+{?\s*Token\s*}?\s+from\s+'../../types'" packages/synapse-constants/src --glob '*.ts' --glob '*.tsx'Length of output: 260
Script:
#!/bin/bash # Description: Verify the consistency of Token import paths across the project # Test: Search for Token imports from types. Expect: All imports should use '../../types' rg "import\\s+\\{?\\s*Token\\s*\\}?\\s+from\\s+'../../types'" packages/synapse-constants/src --glob '*.ts' --glob '*.tsx'Length of output: 711
packages/synapse-constants/src/constants/chains/index.ts (1)
Line range hint
2-138: Confirmed: Rest of the file unaffected by import change.I've reviewed the entire file, and the change in the import path for the
Chaintype does not affect any other part of the code. All usages of theChaintype and other constants and utility functions remain the same and should continue to function as before.packages/synapse-constants/src/types/index.ts (2)
3-4: LGTM: New utility imports added.The new imports for
makeMultiChainObjandvalidateAddressesfrom the../utilsdirectory are correctly added. These utility functions will likely improve code organization and reusability.
Line range hint
1-258: Summary: Successful refactoring of types and Token class.The changes in this file align well with the PR objectives:
- New utility functions are imported, improving code organization.
- The Token class constructor now uses these utilities, enhancing input validation and standardization.
- These modifications contribute to a more maintainable and robust codebase.
Overall, the refactoring appears to be a positive step towards better code quality and organization.
packages/synapse-constants/src/constants/chains/master.ts (1)
Line range hint
1-624: Overall structure looks good.While not part of the changes in this PR, it's worth noting that the file has a well-structured and comprehensive set of chain definitions. This structure should facilitate easy addition of new chains and maintenance of existing ones in the future.
packages/synapse-constants/src/constants/tokens/bridgeable.ts (2)
Line range hint
1-1037: LGTM. Well-structured token definitions.The file contains a comprehensive set of token definitions using the
Tokenclass, which is a good practice for maintaining consistency and ease of use. The definitions include a wide range of tokens across various blockchain networks, promoting interoperability. The structure and content of the file appear to be well-maintained and consistent.
42-42: LGTM. Verify import path consistency across the project.The import path for the
Tokentype has been updated, which likely reflects a change in the project structure. This change looks good, but it's important to ensure that similar updates have been made consistently across the entire project to maintain proper imports.To verify the consistency of this import change across the project, you can run the following script:
| @@ -0,0 +1,13 @@ | |||
| import * as CHAINS from '../constants/chains/master' | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using named imports for better clarity.
Instead of using a wildcard import, it would be more explicit to import only the necessary items from the CHAINS module. This approach enhances code readability and helps in tracking dependencies.
Consider refactoring the import statement to use named imports. For example:
import { ChainId } from '../constants/chains/master'Replace ChainId with the actual types or constants you need from the CHAINS module.
| @@ -0,0 +1,13 @@ | |||
| import * as CHAINS from '../constants/chains/master' | |||
|
|
|||
| export const makeMultiChainObj = (valOrObj: any) => { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more specific type for the valOrObj parameter.
The use of any as a type bypasses TypeScript's type checking, which can lead to potential runtime errors. It's generally better to use more specific types when possible.
Consider refactoring the function signature to use a union type that more accurately represents the expected input. For example:
export const makeMultiChainObj = <T>(valOrObj: T | Record<number, T>) => {
// ...
}This allows valOrObj to be either a value of type T or an object with numeric keys and values of type T.
| import * as CHAINS from '../constants/chains/master' | ||
|
|
||
| export const makeMultiChainObj = (valOrObj: any) => { | ||
| if (typeof valOrObj === 'object') { | ||
| return valOrObj | ||
| } else { | ||
| const obj: { [key: number]: any } = {} | ||
| for (const chain of Object.values(CHAINS)) { | ||
| obj[chain.id] = valOrObj | ||
| } | ||
| return obj | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding type safety, documentation, and null handling.
While the function is well-structured and serves its purpose, there are a few improvements that could enhance its robustness and maintainability:
- Add JSDoc comments to explain the function's purpose, parameters, and return value.
- Handle the case where
valOrObjisnull, which is technically an object in JavaScript but might not be a valid input for this function. - Consider using a generic type parameter for better type inference of the return value.
Here's an example of how you might refactor the function with these improvements:
import { ChainId } from '../constants/chains/master'
/**
* Creates a multi-chain object from a single value or an existing object.
* @template T The type of the value (inferred from input)
* @param {T | Record<number, T> | null} valOrObj - The value or object to use
* @returns {Record<number, T>} An object with chain IDs as keys and the provided value (or values) as values
* @throws {Error} If the input is null
*/
export const makeMultiChainObj = <T>(valOrObj: T | Record<number, T> | null): Record<number, T> => {
if (valOrObj === null) {
throw new Error('Input cannot be null');
}
if (typeof valOrObj === 'object' && !Array.isArray(valOrObj)) {
return valOrObj as Record<number, T>;
} else {
return Object.fromEntries(
Object.values(ChainId).map(chainId => [chainId, valOrObj])
) as Record<number, T>;
}
}This refactored version improves type safety, adds documentation, handles the null case, and uses a more efficient object creation method.
| export const validateAddresses = (addresses: { | ||
| [x: number]: string | ||
| }): { [x: number]: string } => { | ||
| const reformatted: { [x: number]: string } = {} | ||
| for (const chainId in addresses) { | ||
| reformatted[chainId] = addresses[chainId] | ||
| ? getAddress(addresses[chainId]) | ||
| : '' | ||
| } | ||
| return reformatted | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding chain ID validation and error handling.
While the current implementation works well for valid inputs, it might be beneficial to add some additional checks:
- Validate chain IDs: Ensure that the chain IDs are valid (e.g., positive integers).
- Add error handling: Catch and handle potential errors thrown by
getAddressfor invalid addresses.
Here's a suggested implementation with these improvements:
import { getAddress } from '@ethersproject/address'
export const validateAddresses = (addresses: {
[x: number]: string
}): { [x: number]: string } => {
const reformatted: { [x: number]: string } = {}
for (const chainId in addresses) {
const numericChainId = Number(chainId)
if (isNaN(numericChainId) || numericChainId <= 0) {
throw new Error(`Invalid chain ID: ${chainId}`)
}
try {
reformatted[numericChainId] = addresses[chainId]
? getAddress(addresses[chainId])
: ''
} catch (error) {
throw new Error(`Invalid address for chain ID ${chainId}: ${addresses[chainId]}`)
}
}
return reformatted
}This implementation adds chain ID validation and error handling for invalid addresses, providing more robust input validation.
| @@ -1,6 +1,6 @@ | |||
| import usdbLogo from '../assets/icons/usdc.svg' | |||
| import fusdtLogo from '../assets/icons/usdt.svg' | |||
| import { Token } from '../types' | |||
| import { Token } from '../../types' | |||
There was a problem hiding this comment.
💡 Codebase verification
Update Required: Remaining outdated import path found.
The file packages/synapse-constants/src/constants/index.ts still contains an import from '../types'. Please update this import path to '../../types' to maintain consistency with the refactoring.
🔗 Analysis chain
LGTM. Verify import path consistency across the project.
The updated import path for the Token type looks correct. This change aligns with the refactoring mentioned in the PR objectives.
To ensure consistency across the project, please run the following script to check for any remaining imports from '../types':
If the script returns any results, those files may need to be updated to maintain consistency with the new project structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining imports from '../types'
# Expected result: No output, indicating all imports have been updated
rg --type typescript "from '\.\./types'" packages/synapse-constants/src
Length of output: 114
Script:
#!/bin/bash
# Description: Check for any remaining imports from '../types' in TypeScript files
# Expected result: No output, indicating all imports have been updated
rg -t ts "from '\.\./types'" packages/synapse-constants/src
Length of output: 142
| @@ -1,4 +1,4 @@ | |||
| import { Token } from '../types' | |||
| import { Token } from '../../types' | |||
There was a problem hiding this comment.
💡 Codebase verification
Import path inconsistency detected.
The import statement export * from '../types' in packages/synapse-constants/src/constants/index.ts still uses the old path. Please update it to ../../types to ensure consistency across the project.
🔗 Analysis chain
LGTM! Verify import path consistency across the project.
The change in the import path for the Token type is correct and aligns with the PR objective of refactoring the folder structure. However, it's important to ensure that this change is consistent across the entire project.
To verify the consistency of import paths for the Token type across the project, run the following script:
This will help ensure that all relevant files have been updated consistently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining '../types' imports and new '../../types' imports
echo "Checking for any remaining '../types' imports:"
rg --type typescript "from '\.\./types'" packages/synapse-constants/src
echo "Checking for new '../../types' imports:"
rg --type typescript "from '\.\./\.\./types'" packages/synapse-constants/src
Length of output: 438
Script:
#!/bin/bash
# Description: Check for any remaining '../types' imports and new '../../types' imports
echo "Checking for any remaining '../types' imports:"
rg --type ts "from '\.\./types'" packages/synapse-constants/src
echo "Checking for new '../../types' imports:"
rg --type ts "from '\.\./\.\./types'" packages/synapse-constants/src
Length of output: 1203
| @@ -1,4 +1,4 @@ | |||
| import { Chain } from '../types' | |||
| import { Chain } from '../../types' | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Detected: Remaining Import Path Not Updated
The import path from '../types' is still present in packages/synapse-constants/src/constants/index.ts. Please update this import to match the new directory structure to ensure consistency and prevent potential issues.
- File:
packages/synapse-constants/src/constants/index.ts - Line: [specific line number if available]
🔗 Analysis chain
LGTM: Import path updated correctly.
The import path for the Chain type has been updated to reflect the new directory structure. This change is correct and necessary for the proper functioning of the module.
To ensure consistency across the project, please run the following script to check for any other occurrences of the old import path:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining occurrences of the old import path
# Test: Search for '../types'. Expect: No results in this package, except in git history.
rg --type typescript "from '../types'" packages/synapse-constants
Length of output: 108
Script:
#!/bin/bash
# Description: Check for any remaining occurrences of the old import path without specifying file type
# Test: Search for '../types' in TypeScript and JavaScript files. Expect: No results in this package, except in git history.
rg "from '../types'" packages/synapse-constants --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx'
Length of output: 188
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
packages/synapse-constants/src/constants/tokens/auxilliary.ts (2)
11-11: Consider using a production CDN for logo URLsThe logo URL has been changed to an external source, which is good for reducing bundle size. However, the current URL (
https://8f3ea9f2.sanguine-fe.pages.dev/...) appears to be pointing to a development environment.Consider the following:
- Use a production CDN URL to ensure reliability and performance.
- Implement a fallback mechanism in case the external resource becomes unavailable.
- Ensure that using external resources aligns with the project's security policies.
56-56: Standardize logo URL sourcingThe logo URL for MULTIAVAX, like some others, is sourced from a development environment (sanguine-fe.pages.dev). While the consistency in URL structure is good, using a development domain in production could lead to reliability issues.
Consider the following:
- Move all logo assets to a production-ready CDN.
- Implement a centralized configuration for base URL of assets to easily switch between environments.
- Ensure proper caching strategies are in place for these external resources.
packages/synapse-constants/src/constants/chains/master.ts (1)
31-32: Consider the implications of using externally hosted images.All
chainImgproperties have been updated to use direct URLs to hosted SVG images on 'https://8f3ea9f2.sanguine-fe.pages.dev/'. While this change centralizes image hosting and potentially improves load times, it introduces a dependency on an external service. Consider the following:
- Ensure the reliability and uptime of the hosting service.
- Implement a fallback mechanism in case the external service is unavailable.
- Evaluate the impact on offline functionality, if applicable.
- Consider caching strategies to mitigate potential performance issues.
Also applies to: 55-56, 76-77, 98-99, 119-120, 140-141, 161-162, 182-183, 203-204, 224-225, 245-246, 266-267, 287-288, 308-309, 329-330, 350-351, 371-372, 392-393, 414-415, 439-440, 464-465, 489-490
packages/synapse-constants/src/constants/tokens/poolMaster.ts (1)
Line range hint
483-499: Consider removing commented out codeThere's a commented out token definition for
CANTO_USDC_SWAP_TOKEN. If this code is no longer needed, it's better to remove it entirely. Commented out code can clutter the file and may become outdated. If you need to reference this code in the future, you can always retrieve it from version control.If you decide to keep this code, please add a comment explaining why it's being kept and when it might be used in the future.
packages/synapse-constants/src/constants/tokens/bridgeable.ts (3)
Line range hint
359-397: Consider simplifying the USDC token definitionThe
USDCtoken definition contains a large number of chain-specific addresses and decimals. Consider extracting these into a separate configuration object or using a helper function to generate the token definition. This would improve readability and make it easier to maintain in the future.
Line range hint
1-1274: Ensure consistent use ofvisibilityRankacross all tokensThe
visibilityRankproperty is present on some tokens (e.g., USDC, USDT) but not on others. Consider adding this property to all tokens or documenting why certain tokens don't have it. This will improve consistency and make it easier to understand the purpose of this property.
Line range hint
1-1274: AddcoingeckoIdto all applicable tokensSome tokens have a
coingeckoIdproperty, which is useful for integrating with external services. Consider adding this property to all tokens that have a corresponding entry on CoinGecko. This will improve the consistency of the token definitions and potentially enable more features in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- packages/synapse-constants/src/constants/assets/index.ts (1 hunks)
- packages/synapse-constants/src/constants/chains/master.ts (22 hunks)
- packages/synapse-constants/src/constants/tokens/auxilliary.ts (8 hunks)
- packages/synapse-constants/src/constants/tokens/bridgeable.ts (62 hunks)
- packages/synapse-constants/src/constants/tokens/deprecated.ts (3 hunks)
- packages/synapse-constants/src/constants/tokens/poolMaster.ts (34 hunks)
- packages/synapse-constants/src/constants/tokens/sushiMaster.ts (3 hunks)
- packages/synapse-constants/src/constants/tokens/swapMaster.ts (3 hunks)
- packages/synapse-constants/src/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/synapse-constants/src/constants/tokens/deprecated.ts
- packages/synapse-constants/src/constants/tokens/sushiMaster.ts
- packages/synapse-constants/src/constants/tokens/swapMaster.ts
- packages/synapse-constants/src/index.ts
🔇 Additional comments (9)
packages/synapse-constants/src/constants/assets/index.ts (1)
1-1: LGTM! Consider verifying bundle size impact and dependency structure.The export statement effectively consolidates all exports from the './explorer' module, which is a common and clean pattern in TypeScript projects. This approach simplifies imports in other parts of the application.
However, I recommend the following:
- Verify that this change doesn't significantly increase bundle sizes, especially if not all exported items are used in most cases.
- Ensure that this export doesn't introduce any circular dependencies in the project structure.
To check for potential circular dependencies and analyze the impact, you can run the following script:
This script will help identify any potential circular dependencies, show the structure of the explorer module, and reveal how the new index file is being used across the project.
packages/synapse-constants/src/constants/tokens/auxilliary.ts (4)
74-74: Maintain consistency in logo URL structureThe logo URL for FANTOMUSDC follows the same structure as other tokens, which is good for consistency. However, the use of a development domain (sanguine-fe.pages.dev) remains a concern.
As suggested earlier, consider moving all these assets to a production CDN to ensure reliability and performance in the production environment.
92-92: Consistent USDT logo usage across tokensIt's good to see that the same logo URL is used for USDT tokens across different chains (FANTOMUSDT and KLAYTN_oUSDT). This consistency helps maintain a uniform appearance for the same asset type.
However, as mentioned in previous comments, consider moving these assets to a production CDN for improved reliability and performance.
108-108: Consistent approach to logo URL updatesThe logo URL for FANTOMETH follows the same structure as other tokens, maintaining consistency throughout the file. This is a positive aspect of the changes made.
Overall, the modifications in this file have successfully transitioned from local logo imports to external URLs, which can help reduce bundle size and simplify the build process. However, there are a few key points to address:
- Move all logo assets from the development domain (sanguine-fe.pages.dev) to a production-ready CDN.
- Implement a centralized configuration for the base URL of assets.
- Ensure the correct logo is used for each token (e.g., fix the KLAYTN_oUSDT logo URL).
- Consider optimizing the MIM logo URL for better performance and maintainability.
Addressing these points will enhance the reliability and performance of the application in a production environment.
1-1: Verify import path consistency across the projectThe import path for
Tokenhas been updated. Ensure that this change is consistent across all files in the project that import theTokentype.Run the following script to check for any inconsistencies:
✅ Verification successful
Import path consistency verified successfully
No inconsistencies found for the
Tokenimport paths across the project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for inconsistent Token import paths # Test 1: Search for old import path echo "Checking for old import paths:" rg --type typescript "import.*Token.*from '\.\.\/types'" packages/synapse-constants/src # Test 2: Verify new import path usage echo "Verifying new import path usage:" rg --type typescript "import.*Token.*from '\.\./\.\.\/types'" packages/synapse-constants/srcLength of output: 602
Script:
#!/bin/bash # Description: Check for inconsistent Token import paths # Test 1: Search for old import path echo "Checking for old import paths:" rg --type typescript "import\s+.*Token\s+from\s+'\.\./types'" packages/synapse-constants/src # Test 2: Verify new import path usage echo "Verifying new import path usage:" rg --type typescript "import\s+.*Token\s+from\s+'\.\./\.\./types'" packages/synapse-constants/srcLength of output: 422
Script:
#!/bin/bash # Description: Check for inconsistent Token import paths # Test 1: Search for old import path echo "Checking for old import paths:" rg --type ts --type tsx "import\s+.*Token\s+from\s+'\.\./types'" packages/synapse-constants/src # Test 2: Verify new import path usage echo "Verifying new import path usage:" rg --type ts --type tsx "import\s+.*Token\s+from\s+'\.\./\.\./types'" packages/synapse-constants/srcLength of output: 414
Script:
#!/bin/bash # Description: Check for inconsistent Token import paths # Test 1: Search for old import path echo "Checking for old import paths:" rg --glob '*.ts' --glob '*.tsx' "import\s+.*Token\s+from\s+'\.\./types'" packages/synapse-constants/src # Test 2: Verify new import path usage echo "Verifying new import path usage:" rg --glob '*.ts' --glob '*.tsx' "import\s+.*Token\s+from\s+'\.\./\.\./types'" packages/synapse-constants/srcLength of output: 374
Script:
#!/bin/bash # Description: Comprehensive check for inconsistent Token import paths across the entire repository # Test 1: Search for old import path throughout the repository echo "Checking for old import paths across the entire repository:" fd --extension ts --extension tsx --type file | xargs grep -H "import\s\+.*Token\s\+from\s\+'\.\./types'" # Test 2: Verify new import path usage throughout the repository echo "Verifying new import path usage across the entire repository:" fd --extension ts --extension tsx --type file | xargs grep -H "import\s\+.*Token\s\+from\s\+'\.\./\.\./types'"Length of output: 494
packages/synapse-constants/src/constants/tokens/poolMaster.ts (2)
Line range hint
1-910: Remaining changes look goodThe rest of the changes in this file, including the consistent update of the logo URL across all token definitions, appear to be correct and don't introduce any new issues.
1-1: Verify import path change consistencyThe import path for the
Tokentype has been updated. Please ensure that this change is consistent across the entire project and that it doesn't break any other imports.Run the following script to check for any remaining imports from '../types':
✅ Verification successful
Adjust ripgrep command to target TypeScript files
Please run the following script to correctly search for any remaining imports from
'../types'in TypeScript files:
Import path change is consistent across the project
The updated import path for the
Tokentype inpoolMaster.tsaligns correctly with other imports in the codebase, ensuring consistency and preventing potential import issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports from '../types' rg --type typescript "from '../types'" packages/synapse-constants/srcLength of output: 112
Script:
#!/bin/bash # Description: Check for any remaining imports from '../types' in TypeScript files rg "from '../types'" --glob '*.ts' --glob '*.tsx' packages/synapse-constants/srcLength of output: 163
packages/synapse-constants/src/constants/tokens/bridgeable.ts (2)
1-1: LGTM: Import path updated correctlyThe import path for
Tokenhas been updated from '../types' to '../../types', which aligns with the project's restructuring mentioned in the PR objectives.
26-26: LGTM: Logo URLs updated consistentlyThe
logoproperty for all tokens has been updated to use URLs instead of imported assets. This change aligns with the PR objectives and likely improves the build process and reduces the bundle size.Also applies to: 43-43, 59-59, 76-76, 92-92, 110-110, 129-129, 146-146, 165-165, 185-185, 203-203, 220-220, 236-236, 259-259, 276-276, 293-293, 311-311, 331-331, 348-348, 399-399, 431-431, 472-472, 510-510, 539-539, 554-554, 569-569, 601-601, 620-620, 638-638, 665-665, 695-695, 721-721, 756-756, 781-781, 801-801, 818-818, 832-832, 852-852, 868-868, 883-883, 899-899, 918-918, 940-940, 958-958, 981-981, 997-997, 1016-1016, 1033-1033, 1055-1055, 1070-1070, 1090-1090, 1108-1108, 1123-1123, 1140-1140, 1156-1156, 1172-1172, 1196-1196, 1214-1214, 1234-1234, 1254-1254, 1274-1274
| @@ -30,7 +24,7 @@ export const KLAYTN_oUSDT = new Token({ | |||
| }, | |||
| symbol: 'orbitUSDT', | |||
| name: 'Orbit Bridged USDT', | |||
| logo: usdtLogo, | |||
| logo: 'https://8f3ea9f2.sanguine-fe.pages.dev/_next/static/media/usdt.3c9cd2f8.svg', | |||
There was a problem hiding this comment.
Incorrect logo URL for KLAYTN_oUSDT
The logo URL for KLAYTN_oUSDT is currently set to the WETH logo URL. This appears to be an error as USDT should have its own distinct logo.
Please update this to the correct USDT logo URL.
| @@ -45,7 +39,7 @@ | |||
| decimals: 18, | |||
| symbol: 'MIM', | |||
| name: 'Magic Internet Money', | |||
| logo: mimLogo, | |||
| logo: 'https://docs.abracadabra.money/~gitbook/image?url=https%3A%2F%2F2388475231-files.gitbook.io%2F%7E%2Ffiles%2Fv0%2Fb%2Fgitbook-legacy-files%2Fo%2Fassets%252F-Mc9U0yE30Tc9xb3mVGA%252F-McF-MUtKQnjF8iBPrv5%252F-McF8_XaAL4kyHrICXUw%252FMIM%2520Logo%2520PNG.png%3Falt%3Dmedia%26token%3D12b05d37-765f-494d-809b-4deab52bd212&width=768&dpr=2&quality=100&sign=7d2ede98&sv=1', | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider the source of the MIM logo URL
The current MIM logo URL is pointing to a documentation site (docs.abracadabra.money) and contains a long query string. This may not be optimal for performance or long-term maintenance.
Consider the following:
- Host the logo on a dedicated CDN or image hosting service for better performance and reliability.
- Use a shorter, cleaner URL without query parameters if possible.
- Ensure you have permission to use this external resource in your application.
| @@ -42,15 +20,16 @@ import baseExplorerImg from '../assets/explorer/basescan.svg' | |||
| import blastExplorerImg from '../assets/explorer/blast.svg' | |||
| import scrollExplorerImg from '../assets/explorer/scroll.svg' | |||
| import lineaExplorerImg from '../assets/explorer/linea.svg' | |||
| import { Chain } from '../types' | |||
| import { Chain } from '../../types' | |||
There was a problem hiding this comment.
💡 Codebase verification
Update remaining import path from '../types' to '../../types'.
The Chain type import has been updated to '../../types' in packages/synapse-constants/src/constants/chains/master.ts, but the following file still imports from '../types':
packages/synapse-constants/src/constants/index.ts:export * from '../types'
Please revise this import to ensure consistency across the codebase.
🔗 Analysis chain
Verify the updated import path for the Chain type.
The import path for the Chain type has been changed from a relative path to a more absolute one. This change suggests a restructuring of the project's directory structure. Please ensure that this change is consistent across the codebase and that the new path is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the import path for the Chain type across the codebase.
# Search for any remaining '../types' imports
echo "Searching for any remaining '../types' imports:"
rg --type typescript "from '\.\./types'" packages/synapse-constants/src
# List all files importing 'Chain' to ensure they use the correct path
echo "Listing all files importing 'Chain':"
rg --type typescript "import.*Chain.*from" packages/synapse-constants/src
Length of output: 425
Script:
#!/bin/bash
# Description: Verify the import path for the Chain type across the codebase with corrected file type.
# Search for any remaining '../types' imports
echo "Searching for any remaining '../types' imports:"
rg --type ts "from '\.\./types'" packages/synapse-constants/src
# List all files importing 'Chain' to ensure they use the correct path
echo "Listing all files importing 'Chain':"
rg --type ts "import.*Chain.*from" packages/synapse-constants/src
Length of output: 710
| @@ -45,7 +43,7 @@ export const ETH_POOL_SWAP_TOKEN = new Token({ | |||
| decimals: 18, | |||
| symbol: 'nUSD', | |||
| name: 'Synapse nUSD LP Token Ethereum', | |||
| logo: synapseLogo, | |||
| logo: 'https://8f3ea9f2.sanguine-fe.pages.dev/_next/static/media/syn.042f8cd0.svg', | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a constant for the logo URL
The logo URL 'https://8f3ea9f2.sanguine-fe.pages.dev/_next/static/media/syn.042f8cd0.svg' is repeated multiple times throughout the file. To improve maintainability and reduce the risk of typos, consider defining this URL as a constant at the top of the file or in a separate configuration file.
Here's a suggested refactor:
const SYNAPSE_LOGO_URL = 'https://8f3ea9f2.sanguine-fe.pages.dev/_next/static/media/syn.042f8cd0.svg';
// Then use SYNAPSE_LOGO_URL instead of the hardcoded string in all token definitionsAlso applies to: 73-73, 100-100, 125-125, 150-150, 177-177, 203-203, 226-226, 253-253, 280-280, 307-307, 334-334, 359-359, 385-385, 411-411, 437-437, 463-463, 487-487, 510-510, 536-536, 561-561, 588-588, 619-619, 650-650, 681-681, 716-716, 742-742, 768-768, 795-795, 821-821, 846-846, 879-879, 905-905
| @@ -64,7 +23,7 @@ export const GOHM = new Token({ | |||
| decimals: 18, | |||
| symbol: 'gOHM', | |||
| name: 'Olympus DAO', | |||
| logo: ohmLogo, | |||
| logo: 'https://8f3ea9f2.sanguine-fe.pages.dev/_next/static/media/ohm.1b779b45.svg', | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a single base URL for all logo assets
Most logo URLs use the base 'https://8f3ea9f2.sanguine-fe.pages.dev', but some (e.g., AGEUR) use 'https://45a97b3d.sanguine-fe.pages.dev'. For consistency and to reduce potential issues if one domain becomes unavailable, consider using a single base URL for all logo assets.
Also applies to: 43-43, 59-59, 76-76, 92-92, 110-110, 129-129, 146-146, 165-165, 185-185, 203-203, 220-220, 236-236, 259-259, 276-276, 293-293, 311-311, 331-331, 348-348, 399-399, 431-431, 472-472, 510-510, 539-539, 554-554, 569-569, 601-601, 620-620, 638-638, 665-665, 695-695, 721-721, 756-756, 781-781, 801-801, 818-818, 832-832, 852-852, 868-868, 883-883, 899-899, 918-918, 940-940, 958-958, 981-981, 997-997, 1016-1016, 1033-1033, 1055-1055, 1070-1070, 1090-1090, 1108-1108, 1123-1123, 1140-1140, 1156-1156, 1172-1172, 1196-1196, 1214-1214, 1234-1234, 1254-1254, 1274-1274
abtestingalpha
requested review from
bigboydiamonds and
trajan0x
as code owners
Description
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation