Closed
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new non-null
claimsfield onStewardis only initialized inhandleUBIClaim; please ensure every code path that creates aSteward(other mappings/handlers) also sets an initialclaimsvalue to avoid runtime nullability errors in the subgraph. - You added the required
streamedContribution: BigInt!field toDonorCollectivein the schema but there’s no mapping code updating it; consider initializing it onDonorCollectivecreation and updating it wherever stream-related contributions are processed so the subgraph remains consistent. - By removing
CHAIN_OVERRIDESentirely, callers lose the ability to customize gas parameters for specific chains; if some environments still need tuned gas settings, consider making overrides an optional argument to the affected SDK methods rather than hard-coding them or removing them outright.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new non-null `claims` field on `Steward` is only initialized in `handleUBIClaim`; please ensure every code path that creates a `Steward` (other mappings/handlers) also sets an initial `claims` value to avoid runtime nullability errors in the subgraph.
- You added the required `streamedContribution: BigInt!` field to `DonorCollective` in the schema but there’s no mapping code updating it; consider initializing it on `DonorCollective` creation and updating it wherever stream-related contributions are processed so the subgraph remains consistent.
- By removing `CHAIN_OVERRIDES` entirely, callers lose the ability to customize gas parameters for specific chains; if some environments still need tuned gas settings, consider making overrides an optional argument to the affected SDK methods rather than hard-coding them or removing them outright.
## Individual Comments
### Comment 1
<location path="packages/app/package.json" line_range="36-38" />
<code_context>
"@tanstack/react-query": "^5.66.0",
"@uniswap/router-sdk": "^1.9.3",
"@uniswap/sdk-core": "^5.3.1",
- "@uniswap/smart-order-router": "^3.35.12",
+ "@uniswap/smart-order-router": "3.*",
"@uniswap/v3-sdk": "^3.13.1",
"@usedapp/core": "^1.2.10",
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Relaxing the smart-order-router version to `3.*` allows downgrades to older 3.x versions, which may miss APIs you rely on.
`^3.35.12` guaranteed a minimum version with known APIs and fixes. `3.*` permits older 3.x versions that may lack required functionality, particularly for consumers or installs without a reliable lockfile. If you want broader compatibility while preserving the tested floor, use a range like `">=3.35.12 <4"` instead.
```suggestion
"@uniswap/sdk-core": "^5.3.1",
"@uniswap/smart-order-router": ">=3.35.12 <4",
"@uniswap/v3-sdk": "^3.13.1",
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
36
to
38
| "@uniswap/sdk-core": "^5.3.1", | ||
| "@uniswap/smart-order-router": "^3.35.12", | ||
| "@uniswap/smart-order-router": "3.*", | ||
| "@uniswap/v3-sdk": "^3.13.1", |
There was a problem hiding this comment.
suggestion (bug_risk): Relaxing the smart-order-router version to 3.* allows downgrades to older 3.x versions, which may miss APIs you rely on.
^3.35.12 guaranteed a minimum version with known APIs and fixes. 3.* permits older 3.x versions that may lack required functionality, particularly for consumers or installs without a reliable lockfile. If you want broader compatibility while preserving the tested floor, use a range like ">=3.35.12 <4" instead.
Suggested change
| "@uniswap/sdk-core": "^5.3.1", | |
| "@uniswap/smart-order-router": "^3.35.12", | |
| "@uniswap/smart-order-router": "3.*", | |
| "@uniswap/v3-sdk": "^3.13.1", | |
| "@uniswap/sdk-core": "^5.3.1", | |
| "@uniswap/smart-order-router": ">=3.35.12 <4", | |
| "@uniswap/v3-sdk": "^3.13.1", |
… in contracts and SDK, including gas estimation and tests.
… pools with bulk updates and error handling improvements
- Simplified factory addMembers() to inline logic instead of calling external functions - Removed BATCH_TOO_LARGE guards - gas management is now caller's responsibility - Added MANAGER_ROLE access control to pool addMembers() methods - Removed _grantMemberRoleWithoutFactory() helper functions - Consolidated SDK methods into single addPoolMembers() with poolType parameter - Updated tests to remove obsolete double-counting and batch size checks - Fixed SDK TypeScript error in gas estimation function BREAKING CHANGE: SDK addDirectPaymentsPoolMembers() and addUBIPoolMembers() methods removed in favor of consolidated addPoolMembers(poolType) method
…pools - Consolidated member addition logic in addMembers() functions to improve readability and maintainability. - Removed redundant event emissions and replaced them with role grants for better event handling. - Updated tests to reflect changes in event verification and member addition processes. - Adjusted SDK methods to align with the new member addition structure, enhancing usability.
…yments and UBI pools fixing contract build error - Updated addMember functions in DirectPaymentsFactory, UBIPool, and UBIPoolFactory contracts from external to public visibility to enhance accessibility. - This change allows for more flexible member addition while maintaining existing access control mechanisms.
…ctPayments and UBI pools - Updated for-loops in addMembers functions to remove unnecessary unchecked increment statements, enhancing code clarity and maintainability. - This change streamlines the member addition process while preserving existing functionality.
…setup logic - Introduced reusable setup functions for DirectPayments tests to streamline test initialization and improve maintainability. - Updated test cases to utilize the new setup functions, reducing redundancy and enhancing clarity. - Adjusted member validation mocks to ensure accurate testing of member addition scenarios.
…pools - Consolidated member addition functionality by introducing internal helper functions for adding members, improving code clarity and maintainability. - Updated addMembers functions to utilize the new helper methods, enhancing the overall structure of member addition processes.
- Introduced an internal helper function for adding members with validation, improving clarity and maintainability. - Updated the addMembers function to utilize the new helper, allowing for better error handling and skipping invalid members. - Adjusted member validation checks to ensure robust member addition processes while preserving existing functionality.
- Enhanced the addMember function to utilize an internal helper for streamlined member addition and validation. - Improved error handling by determining specific revert reasons based on validation checks. - Updated tests to reflect changes in member validation logic and ensure accurate testing of member addition scenarios.
…r async handling - Changed the mock function call for isMemberValid to await its execution, ensuring accurate test behavior. - This adjustment improves the reliability of the test by correctly handling asynchronous operations.
add: improve gas + subgraph fields
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TODO:
Summary by Sourcery
Relax gas configuration overrides in the GoodCollective SDK and extend subgraph tracking of UBI claims and donor/streamer metrics.
New Features:
Enhancements: