Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system by integrating the new 'pSV' product and 'wSOL' payment token, broadening the range of supported assets. A key architectural improvement is the introduction of global Access Control overrides, which provides a more sophisticated way to manage permissions for individual tokens. To support these new features and improve developer experience, several new deployment and management scripts have been added, simplifying the configuration and ongoing maintenance of token-related functionalities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces support for "pSV Solana" by adding new token configurations, payment token types (wSOL), and mechanisms for global access control overrides. It also includes new scripts for managing these overrides, pausing vault functions, and adding contracts to an address book. The changes involve updates to interfaces, configuration files, deployment scripts, and utility functions to accommodate these new features. Minor refactoring was done in scripts/tasks/manage/token-ac/grant-admin-role.ts and revoke-deployer-roles.ts to make functions reusable.
One area for improvement is the use of hardcoded decimal values (8 and 10 ** 8) in manual feed deployment and price update logic. These should be replaced with named constants for better maintainability and clarity.
| /** | ||
| * Multiplier for converting decimal price strings to base-9 integers | ||
| * Used when converting config prices (e.g., "1.5") to on-chain format | ||
| */ | ||
| export const PRICE_MULTIPLIER = 10 ** PRICE_DECIMALS; | ||
| export const PRICE_MULTIPLIER = 10 ** 9; |
There was a problem hiding this comment.
The removal of PRICE_DECIMALS and hardcoding 10 ** 9 for PRICE_MULTIPLIER reduces maintainability. It's better to keep PRICE_DECIMALS as a constant. Additionally, given that manual feeds now operate with 8 decimals (as seen in scripts/deploy/feeds/manual.ts and scripts/utils/feedDeployment.ts), a new constant, such as MANUAL_FEED_DECIMALS = 8, should be introduced for clarity and consistency.
| /** | |
| * Multiplier for converting decimal price strings to base-9 integers | |
| * Used when converting config prices (e.g., "1.5") to on-chain format | |
| */ | |
| export const PRICE_MULTIPLIER = 10 ** PRICE_DECIMALS; | |
| export const PRICE_MULTIPLIER = 10 ** 9; | |
| /** | |
| * Number of decimal places used for price representation | |
| * Prices are stored as integers with 9 decimal places | |
| * Example: $1.00 is stored as 1_000_000_000 | |
| */ | |
| export const PRICE_DECIMALS = 9; | |
| /** | |
| * Number of decimal places used for manual feed price representation. | |
| */ | |
| export const MANUAL_FEED_DECIMALS = 8; | |
| /** | |
| * Multiplier for converting decimal price strings to base-9 integers | |
| * Used when converting config prices (e.g., "1.5") to on-chain format | |
| */ | |
| export const PRICE_MULTIPLIER = 10 ** PRICE_DECIMALS; |
| const manualFeedTx = new Transaction().add( | ||
| await dataFeedProgram.methods | ||
| .newManualFeed(toBN(initialPrice), PRICE_DECIMALS) | ||
| .newManualFeed(toBN(initialPrice), 8) |
There was a problem hiding this comment.
The hardcoded value 8 for decimals in newManualFeed is a magic number. It should be replaced with the MANUAL_FEED_DECIMALS constant for improved readability and maintainability.
| .newManualFeed(toBN(initialPrice), 8) | |
| .newManualFeed(toBN(initialPrice), MANUAL_FEED_DECIMALS) |
| maxStaleness: dataFeedConfig.maxStaleness, | ||
| initialPrice: dataFeedConfig.initialPrice | ||
| ? BigInt(Math.floor(parseFloat(dataFeedConfig.initialPrice) * PRICE_MULTIPLIER)) | ||
| ? BigInt(Math.floor(parseFloat(dataFeedConfig.initialPrice) * 10 ** 8)) |
There was a problem hiding this comment.
The hardcoded value 10 ** 8 for decimals in the initialPrice calculation is a magic number. It should be replaced with 10 ** MANUAL_FEED_DECIMALS for improved readability and maintainability.
| ? BigInt(Math.floor(parseFloat(dataFeedConfig.initialPrice) * 10 ** 8)) | |
| ? BigInt(Math.floor(parseFloat(dataFeedConfig.initialPrice) * (10 ** MANUAL_FEED_DECIMALS))) |
|
|
||
| // Convert price to base-9 format if provided | ||
| const priceBase9 = price !== null ? toBN(BigInt(Math.round(price * 1e9))) : null; | ||
| const priceBase9 = price !== null ? toBN(BigInt(Math.round(price * 1e8))) : null; |
There was a problem hiding this comment.
The hardcoded value 1e8 for decimals in the priceBase9 calculation is a magic number. It should be replaced with 10 ** MANUAL_FEED_DECIMALS for improved readability and maintainability.
| const priceBase9 = price !== null ? toBN(BigInt(Math.round(price * 1e8))) : null; | |
| const priceBase9 = price !== null ? toBN(BigInt(Math.round(price * (10 ** MANUAL_FEED_DECIMALS)))) : null; |
1 participant
No description provided.