Good Collective bulk add members #311

EmekaManuel · 2025-12-09T04:36:31Z

Bulk Member Addition for DirectPayments and UBI Pools

Description

This PR implements bulk member addition functionality for both DirectPayments and UBI pools, enabling efficient batch operations for adding multiple members in a single transaction. This significantly reduces gas costs and improves the onboarding experience when working with large member lists.

Summary of Changes

🧩 Smart Contracts

DirectPaymentsPool & UBIPool

Added a new function:

addMembers(address[] calldata members, bytes[] calldata extraData)

Includes:

✔️ Uniqueness validation via uniquenessValidator
✔️ Member validation via membersValidator
✔️ Duplicate detection (skips already-registered members)
✔️ Max members limit enforcement (UBI pools only)
✔️ Graceful skipping of invalid members
✔️ MemberAdded event emitted for each successful addition
✔️ MAX_BATCH_SIZE = 200 enforced to avoid gas-limit issues
✔️ Automatic factory registry updates for all valid members

DirectPaymentsFactory & UBIPoolFactory

Added:

addMembers(address[] calldata members)

Functionality includes:

Prevents double-counting using pool-level membership checks
Updates memberPools registry for efficient lookups
Emits MemberAdded events at the factory level

📦 SDK (packages/sdk-js)

Added three new GoodCollectiveSDK methods:

addDirectPaymentsPoolMembers() – Add multiple DirectPayments pool members
addUBIPoolMembers() – Add multiple UBI pool members
estimateBulkAddMembersGas() – Estimate gas + recommend batch sizes

Tests

Comprehensive test suites for both pool types:

DirectPayments.bulkMembers.test.ts
UBIPool.bulkMembers.test.ts

Coverage includes:

✅ Successful bulk addition
✅ Automatic skipping of duplicates
✅ UBI max-members enforcement
✅ Skipping invalid members (uniqueness + custom validation)
✅ Access control
✅ Batch size enforcement (MAX_BATCH_SIZE = 200)
✅ Array length mismatch
✅ Factory registry updates
✅ Gas measurements for batches of 10, 50, 100 members

Key Features

Gas Optimization – Bulk vs. individual additions
Graceful Degradation – Invalid entries skipped
No Double-Counting – Factory registry ensures uniqueness
Full Validation preserved
Event Emission for all successful additions
Type Safety with proper TypeScript support

Breaking Changes

None.

This feature is fully additive and does not modify existing behavior.

How Has This Been Tested?

🧪 Automated Unit Tests

DirectPayments.bulkMembers.test.ts – 10 test cases
UBIPool.bulkMembers.test.ts – 10 test cases (including UBI-specific logic)

Coverage Includes:

Happy paths (valid members)
Edge cases (duplicates, validation failures, access control)
Failure cases (batch size, mismatched arrays, UBI limits)
Integration (factory registry + events)
Gas performance benchmarks

Test Execution:

cd packages/contracts
npx hardhat test test/DirectPayments/DirectPayments.bulkMembers.test.ts
npx hardhat test test/UBIPool/UBIPool.bulkMembers.test.ts

🧪 Manual Testing

Verified on local Hardhat network
Gas usage within acceptable limits (< 30M block gas)
Factory registry updates confirmed

… in contracts and SDK, including gas estimation and tests.

…at-toolbox.

sourcery-ai

Hey there - I've reviewed your changes - here's some feedback:

In both UBIPool.addMembers and DirectPaymentsPool.addMembers, consider replacing the generic revert("Length mismatch") with a named custom error (e.g. error LENGTH_MISMATCH();) for gas savings and consistency with the other custom errors.
In estimateBulkAddMembersGas in the SDK, you compute gasPerMember as gasEstimate.div(members.length); it would be safer to explicitly handle the members.length === 0 case to avoid division by zero and to return a well-defined recommendedBatchSize for empty input.
The new addMembers functions on DirectPaymentsFactory and UBIPoolFactory are not invoked by the new bulk add paths (which still go through _grantRole → addMember); if the intent is to centralize duplicate-prevention at the factory level, consider wiring the pools to call addMembers for bulk updates or clarifying why both variants are needed.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In both `UBIPool.addMembers` and `DirectPaymentsPool.addMembers`, consider replacing the generic `revert("Length mismatch")` with a named custom error (e.g. `error LENGTH_MISMATCH();`) for gas savings and consistency with the other custom errors.
- In `estimateBulkAddMembersGas` in the SDK, you compute `gasPerMember` as `gasEstimate.div(members.length)`; it would be safer to explicitly handle the `members.length === 0` case to avoid division by zero and to return a well-defined `recommendedBatchSize` for empty input.
- The new `addMembers` functions on `DirectPaymentsFactory` and `UBIPoolFactory` are not invoked by the new bulk add paths (which still go through `_grantRole` → `addMember`); if the intent is to centralize duplicate-prevention at the factory level, consider wiring the pools to call `addMembers` for bulk updates or clarifying why both variants are needed.

## Individual Comments

### Comment 1
<location> `packages/sdk-js/src/goodcollective/goodcollective.ts:855-864` </location>
<code_context>
+    const pool = poolType === 'ubi' ? this.ubipool.attach(poolAddress) : this.pool.attach(poolAddress);
+
+    // Estimate gas for the transaction
+    const gasEstimate = await pool.estimateGas.addMembers(members, extraData);
+
+    // Get current gas price
+    const gasPrice = await pool.provider.getGasPrice();
+
+    // Calculate native token required
+    const nativeTokenRequired = gasEstimate.mul(gasPrice);
+
+    // Calculate recommended batch size based on gas estimate
+    // Assuming block gas limit of 30M and targeting 50% usage for safety
+    const targetGasLimit = 15000000;
+    const gasPerMember = gasEstimate.div(members.length);
+    const recommendedBatchSize = Math.min(200, Math.floor(targetGasLimit / gasPerMember.toNumber()));
+
+    return {
</code_context>

<issue_to_address>
**issue (bug_risk):** Guard against empty members array and avoid BigNumber.toNumber() for gas arithmetic.

`members.length === 0` will cause `gasEstimate.div(members.length)` to throw. Also, `gasPerMember.toNumber()` risks exceeding JS’s safe integer range for large gas values. Consider short‑circuiting the empty‑array case (e.g., return zeros and batch size 0), and keep calculations in `BigNumber` until the end (e.g., `targetGasLimitBN.div(gasPerMember)` then clamp before converting to a JS number).
</issue_to_address>

### Comment 2
<location> `packages/sdk-js/src/goodcollective/goodcollective.ts:867` </location>
<code_context>
+    // Assuming block gas limit of 30M and targeting 50% usage for safety
+    const targetGasLimit = 15000000;
+    const gasPerMember = gasEstimate.div(members.length);
+    const recommendedBatchSize = Math.min(200, Math.floor(targetGasLimit / gasPerMember.toNumber()));
+
+    return {
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Avoid duplicating the MAX_BATCH_SIZE constant value in the SDK to prevent drift.

The SDK currently hardcodes `200` when calculating `recommendedBatchSize`, while the contracts define `MAX_BATCH_SIZE = 200`. If the contract limit changes, the SDK will become inconsistent. Consider deriving this value from the contract (e.g., a view/static property) or centralizing the constant in the SDK so it stays in sync.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

packages/sdk-js/src/goodcollective/goodcollective.ts

openzeppelin-code · 2025-12-09T04:40:01Z

Good Collective bulk add members

Generated at commit: 83e9e122a5c890b67273fb042157797a98b72032

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	1 1 0 4 29 35
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

… pools with bulk updates and error handling improvements

packages/contracts/contracts/DirectPayments/DirectPaymentsFactory.sol

packages/contracts/contracts/DirectPayments/DirectPaymentsPool.sol

packages/contracts/contracts/UBI/UBIPool.sol

sirpy · 2025-12-14T07:41:49Z

packages/contracts/test/DirectPayments/DirectPayments.bulkMembers.test.ts

+        };
+    });
+
+    const fixture = async () => {


dont duplicate code. import from existing tests

you didnt reduce duplicated code here

still didnt address this comment

packages/contracts/test/DirectPayments/DirectPayments.bulkMembers.test.ts

packages/sdk-js/src/goodcollective/goodcollective.ts

- 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

EmekaManuel · 2025-12-15T11:59:38Z

hi @sirpy, kindly review. I have fixed all the review issues - e4add4e

sirpy · 2025-12-24T08:17:52Z

packages/contracts/contracts/DirectPayments/DirectPaymentsPool.sol

    );
    event NFTClaimed(uint256 indexed tokenId, uint256 totalRewards);
    event NOT_MEMBER_OR_WHITELISTED_OR_LIMITS(address contributer);
    event MemberAdded(address indexed member);


no need for this event. the grantrole emits an event

sirpy · 2025-12-24T08:18:13Z

packages/contracts/contracts/DirectPayments/DirectPaymentsPool.sol

+            if (added) {
+                emit MemberAdded(members[i]);


packages/contracts/contracts/DirectPayments/DirectPaymentsFactory.sol

packages/contracts/contracts/UBI/UBIPool.sol

sirpy · 2025-12-24T08:25:17Z

packages/contracts/contracts/UBI/UBIPoolFactory.sol

-                emit MemberAdded(member, msg.sender);
-            }
-
+            memberPools[members[i]].push(msg.sender);


there's already an addMember method

packages/contracts/test/DirectPayments/DirectPayments.bulkMembers.test.ts

packages/contracts/test/UBIPool/UBIPool.bulkMembers.test.ts

packages/sdk-js/src/goodcollective/goodcollective.ts

…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.

EmekaManuel · 2025-12-24T21:02:35Z

kindly review @sirpy

sirpy · 2025-12-29T08:08:19Z

packages/contracts/contracts/DirectPayments/DirectPaymentsPool.sol

+        for (uint i = 0; i < members.length; ) {
+            _addMember(members[i], extraData[i]);
+            unchecked {
+                ++i;


why aren't you using the regular for loop format?

sirpy · 2025-12-29T08:08:55Z

packages/contracts/contracts/DirectPayments/DirectPaymentsPool.sol

 import { IERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
-import { IERC721ReceiverUpgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721ReceiverUpgradeable.sol";
+import {
+    IERC721ReceiverUpgradeable


what is this?

sirpy · 2025-12-29T08:09:08Z

packages/contracts/contracts/UBI/UBIPool.sol

 import { IERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
-import { IERC721ReceiverUpgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721ReceiverUpgradeable.sol";
+import {
+    IERC721ReceiverUpgradeable


what is this?

sirpy · 2025-12-29T08:09:43Z

packages/contracts/contracts/UBI/UBIPool.sol

+
+        for (uint i = 0; i < members.length; ) {
+            addMember(members[i], extraData[i]);
+            unchecked {


why not use regular for loop format?

sirpy · 2025-12-29T08:09:53Z

packages/contracts/contracts/UBI/UBIPoolFactory.sol

+    function addMembers(address[] calldata members) external onlyPool {
+        for (uint i = 0; i < members.length; ) {
+            addMember(members[i]);
+            unchecked {


regular for loop

sirpy · 2025-12-29T08:10:12Z

packages/contracts/test/DirectPayments/DirectPayments.bulkMembers.test.ts

+        };
+    });
+
+    const fixture = async () => {


still didnt address this comment

sirpy · 2025-12-29T08:13:00Z

@EmekaManuel please go over previous comments.
Also please reply for each comment with fixed or not fixed + reason, so I know you've seen it.

EmekaManuel added 2 commits December 9, 2025 05:23

feat: Implement bulk member addition for DirectPayments and UBI pools…

cacf984

… in contracts and SDK, including gas estimation and tests.

refactor: Add type assertions for contract instances and import hardh…

5a0defe

…at-toolbox.

EmekaManuel requested a deployment to external December 9, 2025 04:36 — with GitHub Actions Waiting

EmekaManuel mentioned this pull request Dec 9, 2025

GoodCollective pools should support BulkAdd members #307

Open

3 tasks

sourcery-ai bot reviewed Dec 9, 2025

View reviewed changes

packages/sdk-js/src/goodcollective/goodcollective.ts Outdated Show resolved Hide resolved

packages/sdk-js/src/goodcollective/goodcollective.ts Outdated Show resolved Hide resolved

feat: Enhance member addition functionality in DirectPayments and UBI…

15c42c0

… pools with bulk updates and error handling improvements

EmekaManuel requested a deployment to external December 10, 2025 08:06 — with GitHub Actions Waiting

sirpy requested changes Dec 14, 2025

View reviewed changes

EmekaManuel requested a deployment to external December 15, 2025 11:56 — with GitHub Actions Waiting

EmekaManuel requested a review from sirpy December 15, 2025 11:59

L03TJ3 linked an issue Dec 23, 2025 that may be closed by this pull request

GoodCollective pools should support BulkAdd members #307

Open

3 tasks

sirpy requested changes Dec 24, 2025

View reviewed changes

EmekaManuel requested a deployment to external December 24, 2025 21:01 — with GitHub Actions Waiting

sirpy requested changes Dec 29, 2025

View reviewed changes

Good Collective bulk add members #311

Are you sure you want to change the base?

Good Collective bulk add members #311

Uh oh!

Conversation

EmekaManuel commented Dec 9, 2025 • edited by L03TJ3 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bulk Member Addition for DirectPayments and UBI Pools

Description

Summary of Changes

🧩 Smart Contracts

DirectPaymentsPool & UBIPool

DirectPaymentsFactory & UBIPoolFactory

📦 SDK (packages/sdk-js)

Tests

Key Features

Breaking Changes

How Has This Been Tested?

🧪 Automated Unit Tests

🧪 Manual Testing

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openzeppelin-code bot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Good Collective bulk add members

🚨 Report Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EmekaManuel commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EmekaManuel commented Dec 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirpy commented Dec 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

EmekaManuel commented Dec 9, 2025 •

edited by L03TJ3

Loading

openzeppelin-code bot commented Dec 9, 2025 •

edited

Loading

EmekaManuel commented Dec 15, 2025 •

edited

Loading