Good Collective bulk add members by EmekaManuel · Pull Request #311 · GoodDollar/GoodCollective

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: 07e2ae17c38fcde07d3e55e2c3fe4f76f5dd99da

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	1 1 0 4 28 34
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

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

packages/contracts/contracts/DirectPayments/DirectPaymentsPool.sol

packages/contracts/contracts/DirectPayments/DirectPaymentsFactory.sol

packages/contracts/contracts/UBI/UBIPool.sol

packages/contracts/contracts/UBI/UBIPoolFactory.sol

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

packages/contracts/contracts/DirectPayments/DirectPaymentsPool.sol

packages/contracts/contracts/UBI/UBIPool.sol

packages/contracts/contracts/UBI/UBIPoolFactory.sol

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

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.

sirpy · 2026-01-06T09:19:29Z

@EmekaManuel ping

sirpy

Hey @EmekaManuel
This has been inactive for over two weeks.
You could be removed from this bounty if you don't complete it.

sirpy · 2026-01-13T10:44:55Z

@L03TJ3 CC

L03TJ3 · 2026-01-13T10:47:49Z

@sirpy, yes, agree, let's give it till Thursday.
So far Got reminders here, in telegram public channel (last week), and a DM (today)

EmekaManuel · 2026-01-13T12:19:58Z

Hey @EmekaManuel This has been inactive for over two weeks. You could be removed from this bounty if you don't complete it.

so sorry @L03TJ3 Lewis and @sirpy, been sick for a while, just recovering. Let me take a look and address this now

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

sirpy · 2026-01-21T11:34:04Z

@EmekaManuel see the unit tests that are failing

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

EmekaManuel · 2026-01-22T09:16:51Z

hi @sirpy i've gone through these tests, and I still get them failing, can i please get some assistance ?

sirpy · 2026-01-26T10:56:19Z

hi @sirpy i've gone through these tests, and I still get them failing, can i please get some assistance ?
The tests should pass for you locally on your dev env
What kind of error are you getting? From the last run here It looks like some bad input you are passing to one of the contract calls, probably an undefined address or something

sirpy · 2026-01-26T10:50:28Z

packages/contracts/contracts/UBI/UBIPool.sol

+     * @param extraData Additional data to validate the member.
+     */
+    function addMember(address member, bytes memory extraData) public returns (bool isMember) {
        if (address(settings.uniquenessValidator) != address(0)) {


this is now duplicate code. why isnt it calling _addmember?

L03TJ3 · 2026-02-04T03:53:05Z

@EmekaManuel Hey pinging to see if any progress is being done at the moment?

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

sirpy · 2026-02-08T09:23:07Z

packages/contracts/contracts/DirectPayments/DirectPaymentsFactory.sol

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


should call the internal _addmembertoregistry

sirpy · 2026-02-08T09:28:46Z

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

+        // For uniqueness validator, set a default that returns the input address (indicating valid)
+        // Note: Mock will return zero address for unmocked calls, so we use a workaround
+        // Set up a reverting default that we'll override per address as needed
+        uniquenessValidator.mock["getWhitelistedRoot"].returns(ethers.constants.AddressZero.slice(0, -1) + "1");


what is this slice for?

For uniqueness validator, default return must be non-zero (valid/whitelisted). Waffle mock returns zero for unmocked calls; use a non-zero default so we only need to mock addresses we want to treat as invalid (return zero).

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 had a problem deploying to external December 9, 2025 04:36 — with GitHub Actions Failure

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 had a problem deploying to external December 10, 2025 08:06 — with GitHub Actions Failure

sirpy requested changes Dec 14, 2025

View reviewed changes

EmekaManuel had a problem deploying to external December 15, 2025 11:56 — with GitHub Actions Failure

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 had a problem deploying to external December 24, 2025 21:01 — with GitHub Actions Failure

sirpy requested changes Dec 29, 2025

View reviewed changes

sirpy self-requested a review January 13, 2026 10:43

sirpy requested changes Jan 13, 2026

View reviewed changes

EmekaManuel requested a deployment to external January 13, 2026 12:31 — with GitHub Actions Waiting

EmekaManuel requested a deployment to external January 13, 2026 13:09 — with GitHub Actions Waiting

sirpy approved these changes Jan 20, 2026

View reviewed changes

EmekaManuel requested a deployment to external January 22, 2026 08:36 — with GitHub Actions Waiting

EmekaManuel requested a review from sirpy January 22, 2026 08:58

sirpy requested changes Jan 26, 2026

View reviewed changes

EmekaManuel requested a deployment to external February 5, 2026 08:13 — with GitHub Actions Waiting

EmekaManuel force-pushed the gc-bulk-add-members branch from b25d8f6 to c8c9b7d Compare February 5, 2026 09:30

EmekaManuel requested a deployment to external February 5, 2026 09:30 — with GitHub Actions Waiting

EmekaManuel requested a deployment to external February 5, 2026 09:53 — with GitHub Actions Waiting

EmekaManuel requested a deployment to external February 5, 2026 09:58 — with GitHub Actions Waiting

EmekaManuel requested a review from sirpy February 5, 2026 10:00

chore(): eliminated duplicated code

c0deb2c

EmekaManuel requested a deployment to external February 5, 2026 10:46 — with GitHub Actions Waiting

EmekaManuel requested a deployment to external February 5, 2026 10:51 — with GitHub Actions Waiting

EmekaManuel requested a deployment to external February 5, 2026 11:12 — with GitHub Actions Waiting

EmekaManuel requested a deployment to external February 5, 2026 11:38 — with GitHub Actions Waiting

EmekaManuel force-pushed the gc-bulk-add-members branch from 85c7ca5 to c0deb2c Compare February 5, 2026 13:31

EmekaManuel requested a deployment to external February 5, 2026 13:31 — with GitHub Actions Waiting

feat: fix feedback issues

f571d28

EmekaManuel requested a deployment to external February 5, 2026 16:03 — with GitHub Actions Waiting

sirpy requested changes Feb 8, 2026

View reviewed changes

refactor: update member addition logic in DirectPaymentsFactory

07e2ae1

EmekaManuel requested a deployment to external February 8, 2026 12:12 — with GitHub Actions Waiting

EmekaManuel requested a review from sirpy February 9, 2026 00:10

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EmekaManuel commented Dec 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sirpy commented Dec 29, 2025

Uh oh!

sirpy commented Jan 6, 2026

Uh oh!

sirpy left a comment

Choose a reason for hiding this comment

Uh oh!

sirpy commented Jan 13, 2026

Uh oh!

L03TJ3 commented Jan 13, 2026

Uh oh!

EmekaManuel commented Jan 13, 2026

Uh oh!

sirpy commented Jan 21, 2026

Uh oh!

EmekaManuel commented Jan 22, 2026

Uh oh!

sirpy commented Jan 26, 2026

Uh oh!

sirpy Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

EmekaManuel Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

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