feat: implement bulk member addition for DirectPayments and UBI pools#313
Open
EmekaManuel wants to merge 13 commits intoGoodDollar:masterfrom
Open
feat: implement bulk member addition for DirectPayments and UBI pools#313EmekaManuel wants to merge 13 commits intoGoodDollar:masterfrom
EmekaManuel wants to merge 13 commits intoGoodDollar:masterfrom
Conversation
… 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.
- Added `addMembers` function to `DirectPaymentsFactory` and `UBIPoolFactory` for adding multiple members in a single transaction. - Introduced `MemberAdded` event to track successful additions of members. - Enhanced `DirectPaymentsPool` and `UBIPool` contracts with validation and handling for bulk member additions. - Updated `GoodCollectiveSDK` to include methods for adding members to both pool types and estimating gas for bulk operations. - Added comprehensive tests for bulk member addition functionality in both DirectPayments and UBI pools to ensure reliability and performance.
![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 6 issues, and left some high level feedback:
- In
estimateBulkAddMembersGasyou type the first parameter asSigner | Providerbut then unconditionally cast toSignerbefore callingestimateGas, which will fail at runtime when a plain provider is passed; either narrow the type toSigneror branch on the instance type and use a provider-connected contract for estimation. - The new
MemberAddedevents inDirectPaymentsFactoryandUBIPoolFactoryare never emitted in the newaddMembersfunctions; if the event is intended to track registry updates, consider emitting it insideaddMembers(and optionally guarding against duplicatememberPoolsentries for the same pool/member pair to keep the registry compact).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `estimateBulkAddMembersGas` you type the first parameter as `Signer | Provider` but then unconditionally cast to `Signer` before calling `estimateGas`, which will fail at runtime when a plain provider is passed; either narrow the type to `Signer` or branch on the instance type and use a provider-connected contract for estimation.
- The new `MemberAdded` events in `DirectPaymentsFactory` and `UBIPoolFactory` are never emitted in the new `addMembers` functions; if the event is intended to track registry updates, consider emitting it inside `addMembers` (and optionally guarding against duplicate `memberPools` entries for the same pool/member pair to keep the registry compact).
## Individual Comments
### Comment 1
<location> `packages/sdk-js/src/goodcollective/goodcollective.ts:750-751` </location>
<code_context>
+ const connected = pool.attach(poolAddress);
+
+ // Estimate gas for sample batch
+ const estimatedGas = await connected
+ .connect(signerOrProvider as ethers.Signer)
+ .estimateGas.addMembers(sampleMembers, sampleExtraData);
+
</code_context>
<issue_to_address>
**issue:** Casting a Provider to `Signer` will fail when a provider is passed to `estimateBulkAddMembersGas`.
This function takes `ethers.Signer | ethers.providers.Provider` but then casts `signerOrProvider` to `ethers.Signer` before calling `connect`. If a provider is passed, that cast is incorrect. Since `Contract.connect` already accepts both `Signer` and `Provider`, you can drop the cast and call `connected.connect(signerOrProvider)` directly, or better, construct `pool` with the correct signer/provider and avoid reconnecting here.
</issue_to_address>
### Comment 2
<location> `packages/sdk-js/src/goodcollective/goodcollective.ts:759-761` </location>
<code_context>
+ // Conservative batch size calculation
+ // Target ~10M gas per batch to stay well under block gas limit
+ const targetGasPerBatch = 10_000_000;
+ const recommendedBatchSize = Math.min(
+ Math.floor(targetGasPerBatch / estimatedGasPerMember.toNumber()),
+ 200 // MAX_BATCH_SIZE
+ );
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `BigNumber.toNumber()` for gas math risks overflow and unnecessary JS number assumptions.
Here `estimatedGasPerMember.toNumber()` is used in `targetGasPerBatch / estimatedGasPerMember.toNumber()`. This is brittle and can overflow if gas estimates are larger than expected. Prefer keeping the math in `BigNumber` by doing `BigNumber.from(targetGasPerBatch).div(estimatedGasPerMember)` and only converting to a JS number for the final clamped result (e.g. `Math.min(bnValue.toNumber(), 200)`).
Suggested implementation:
```typescript
// Conservative batch size calculation
// Target ~10M gas per batch to stay well under block gas limit
const targetGasPerBatch = ethers.BigNumber.from(10_000_000);
const recommendedBatchSizeBn = targetGasPerBatch.div(estimatedGasPerMember);
const recommendedBatchSize = Math.min(
recommendedBatchSizeBn.toNumber(),
200 // MAX_BATCH_SIZE
);
```
1. Ensure `ethers` is already imported in this file (it appears to be, since `ethers.Signer` is used). If not, add `import { ethers } from "ethers";` at the top.
2. If there is a centralized constant for `MAX_BATCH_SIZE` elsewhere in the file, replace the inline `200` with that constant to keep config consistent.
</issue_to_address>
### Comment 3
<location> `packages/contracts/contracts/DirectPayments/DirectPaymentsPool.sol:231-222` </location>
<code_context>
super._revokeRole(role, account);
}
+ function addMembers(address[] calldata members, bytes[] calldata extraData) external {
+ if (members.length != extraData.length || members.length > 200) revert INVALID_INPUT();
+
+ address[] memory addedMembers = new address[](members.length);
+ uint256 addedCount;
+
+ for (uint256 i; i < members.length; ++i) {
+ if (_addMemberWithoutRegistry(members[i], extraData[i])) {
+ addedMembers[addedCount++] = members[i];
+ }
+ }
+
</code_context>
<issue_to_address>
**🚨 issue (security):** Public `addMembers` on DirectPaymentsPool has no access control and allows arbitrary callers to add members.
Unlike the UBI pool’s bulk entrypoint, this one relies only on validators/uniqueness checks, so any address can repeatedly attempt to add arbitrary members (griefing, spam) and effectively shifts admission control to validator configuration instead of pool governance. Please align this function’s access control with the single-member path (e.g., `onlyRole(MANAGER_ROLE)` or equivalent) so bulk and single additions are governed by the same permissions model.
</issue_to_address>
### Comment 4
<location> `packages/contracts/test/UBIPool/UBIPool.bulkMembers.test.ts:512-516` </location>
<code_context>
+ });
+
+ describe('addMembers - Error Cases', () => {
+ it('should revert if arrays have mismatched lengths', async () => {
+ const members = [signers[1].address, signers[2].address];
+ const extraData = ['0x']; // Only 1 element
+
+ await expect(pool.addMembers(members, extraData)).to.be.reverted;
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Assert on the specific revert reason for better regression protection
This test only asserts that `addMembers` reverts, not *why* it reverts. If the contract uses a specific revert message or custom error for length mismatch, please assert it via `.to.be.revertedWith(...)` or `.to.be.revertedWithCustomError(...)` so changes in revert semantics are caught by the test.
Suggested implementation:
```typescript
describe('addMembers - Error Cases', () => {
it('should revert with length mismatch error when arrays have mismatched lengths', async () => {
const members = [signers[1].address, signers[2].address];
const extraData = ['0x']; // Only 1 element
await expect(pool.addMembers(members, extraData)).to.be.revertedWith(
'UBIPool: members and extraData length mismatch',
);
});
});
type SignerWithAddress = Awaited<ReturnType<typeof ethers.getSigner>>;
```
If the actual revert semantics differ, you should adjust the expectation accordingly:
1. If the contract uses a different revert string, replace `'UBIPool: members and extraData length mismatch'` with the actual message.
2. If the contract uses a custom error instead of a revert string, replace `.to.be.revertedWith(...)` with `.to.be.revertedWithCustomError(pool, 'YourCustomErrorName')`.
</issue_to_address>
### Comment 5
<location> `packages/contracts/test/UBIPool/UBIPool.bulkMembers.test.ts:519-516` </location>
<code_context>
+ await expect(pool.addMembers(members, extraData)).to.be.reverted;
+ });
+
+ it('should revert if batch size exceeds MAX_BATCH_SIZE (200)', async () => {
+ const members: string[] = [];
+ const extraData: string[] = [];
+
+ // Try to add 201 members
+ for (let i = 0; i < 201; i++) {
+ const wallet = ethers.Wallet.createRandom();
+ members.push(wallet.address);
+ extraData.push('0x');
+ }
+
+ await expect(pool.addMembers(members, extraData)).to.be.reverted;
+ });
+ });
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen MAX_BATCH_SIZE test by asserting the exact revert reason
To better validate the batch-size guard, assert the specific revert reason here (e.g., `.to.be.revertedWith('...')` or `.to.be.revertedWithCustomError(...)`) rather than just `.to.be.reverted`. Otherwise a different revert path (like gas issues or a new pre-check) could still make this test pass without confirming the intended condition.
Suggested implementation:
```typescript
it('should revert if batch size exceeds MAX_BATCH_SIZE (200)', async () => {
const members: string[] = [];
const extraData: string[] = [];
// Try to add 201 members
for (let i = 0; i < 201; i++) {
const wallet = ethers.Wallet.createRandom();
members.push(wallet.address);
extraData.push('0x');
}
// Assert the specific revert reason to ensure we're hitting the MAX_BATCH_SIZE guard
await expect(pool.addMembers(members, extraData))
.to.be.revertedWith('MAX_BATCH_SIZE_EXCEEDED');
// If the contract instead uses a custom error, replace the line above with:
// await expect(pool.addMembers(members, extraData))
// .to.be.revertedWithCustomError(pool, 'MaxBatchSizeExceeded');
});
```
` that you should replace it with the actual revert reason or custom error.
Here are the edits:
```xml
<file_operations>
<file_operation operation="edit" file_path="packages/contracts/test/UBIPool/UBIPool.bulkMembers.test.ts">
<<<<<<< SEARCH
it('should revert if batch size exceeds MAX_BATCH_SIZE (200)', async () => {
const members: string[] = [];
const extraData: string[] = [];
// Try to add 201 members
for (let i = 0; i < 201; i++) {
const wallet = ethers.Wallet.createRandom();
members.push(wallet.address);
extraData.push('0x');
}
await expect(pool.addMembers(members, extraData)).to.be.reverted;
});
=======
it('should revert if batch size exceeds MAX_BATCH_SIZE (200)', async () => {
const members: string[] = [];
const extraData: string[] = [];
// Try to add 201 members
for (let i = 0; i < 201; i++) {
const wallet = ethers.Wallet.createRandom();
members.push(wallet.address);
extraData.push('0x');
}
// Assert the specific revert reason to ensure we're hitting the MAX_BATCH_SIZE guard
await expect(pool.addMembers(members, extraData))
.to.be.revertedWith('MAX_BATCH_SIZE_EXCEEDED');
// If the contract instead uses a custom error, replace the line above with:
// await expect(pool.addMembers(members, extraData))
// .to.be.revertedWithCustomError(pool, 'MaxBatchSizeExceeded');
});
>>>>>>> REPLACE
</file_operation>
</file_operations>
```
<additional_changes>
1. Replace `'MAX_BATCH_SIZE_EXCEEDED'` with the actual revert string used in the UBIPool contract for the batch-size check, if it uses `require(..., "message")`.
2. If the contract uses a custom error instead (for example `error MaxBatchSizeExceeded();`), delete the `.revertedWith(...)` call and switch to the `.revertedWithCustomError(pool, 'MaxBatchSizeExceeded')` version shown in the comment.
3. After adjusting the revert reason/custom error name, remove the inline commented alternative to keep the test file clean and consistent with the rest of the codebase.
</issue_to_address>
### Comment 6
<location> `packages/contracts/test/DirectPayments/DirectPayments.bulkMembers.test.ts:306-303` </location>
<code_context>
+ await expect(pool.addMembers(members, extraData)).to.be.reverted;
+ });
+
+ it('should revert if batch size exceeds MAX_BATCH_SIZE (200)', async () => {
+ const members: string[] = [];
+ const extraData: string[] = [];
+
+ // Try to add 201 members
+ for (let i = 0; i < 201; i++) {
+ const wallet = ethers.Wallet.createRandom();
+ members.push(wallet.address);
+ extraData.push('0x');
+ }
+
+ await expect(pool.addMembers(members, extraData)).to.be.reverted;
+ });
+ });
</code_context>
<issue_to_address>
**suggestion (testing):** Tighten MAX_BATCH_SIZE test with explicit revert message/custom error
For the DirectPayments pool tests, please also assert the specific revert reason/custom error when `MAX_BATCH_SIZE` is exceeded. Using only `to.be.reverted` allows unrelated failures to pass the test and can hide regressions in the batch-size check.
Suggested implementation:
```typescript
it('should revert if batch size exceeds MAX_BATCH_SIZE (200)', async () => {
const members: string[] = [];
const extraData: string[] = [];
// Try to add 201 members
for (let i = 0; i < 201; i++) {
const wallet = ethers.Wallet.createRandom();
members.push(wallet.address);
extraData.push('0x');
}
await expect(pool.addMembers(members, extraData))
.to.be.revertedWithCustomError(pool, 'MaxBatchSizeExceeded');
});
```
1. Replace `'MaxBatchSizeExceeded'` with the actual custom error name emitted by the DirectPayments pool when the batch size exceeds `MAX_BATCH_SIZE`. If the contract uses a `require` with a revert string instead of a custom error, change the assertion to:
```ts
await expect(pool.addMembers(members, extraData))
.to.be.revertedWith('YourExactRevertMessageHere');
```
2. Ensure the DirectPayments contract is compiled with the custom error and that the test suite already uses `.to.be.revertedWithCustomError` elsewhere (for consistency). If not, you may want to update similar tests to also assert specific revert reasons.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
packages/contracts/test/DirectPayments/DirectPayments.bulkMembers.test.ts
Outdated
Show resolved
Hide resolved
feat: implement bulk member addition for DirectPayments and UBI pools
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
3 tasks
Contributor
Author
|
kindly review @sirpy |
- Updated `addMembers` function to include role validation for managers, allowing them to bypass certain checks. - Modified `_addMemberWithoutRegistry` to accept a new parameter for manager status, refining member validation logic. - Adjusted tests to ensure proper handling of member addition based on role, including scenarios for non-manager callers. - Improved error handling in tests to provide clearer feedback on invalid input cases.
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
1 participant
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.
addMembersfunction toDirectPaymentsFactoryandUBIPoolFactoryfor adding multiple members in a single transaction.MemberAddedevent to track successful additions of members.DirectPaymentsPoolandUBIPoolcontracts with validation and handling for bulk member additions.GoodCollectiveSDKto include methods for adding members to both pool types and estimating gas for bulk operations.Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
About # (link your issue here)
#307
How Has This Been Tested?
All unit tests passed -
Please describe the tests that you ran to verify your changes.
Checklist:
Summary by Sourcery
Add bulk member addition capabilities to DirectPayments and UBI pools and expose them via the SDK, including gas estimation utilities.
New Features:
Enhancements:
Tests: