-
Notifications
You must be signed in to change notification settings - Fork 20
Good Collective bulk add members #311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
cacf984
5a0defe
15c42c0
e4add4e
83e9e12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,9 @@ import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils | |
| import { IERC721Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721Upgradeable.sol"; | ||
| import { SafeERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol"; | ||
| import { IERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol"; | ||
| import { IERC721ReceiverUpgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721ReceiverUpgradeable.sol"; | ||
| import { | ||
| IERC721ReceiverUpgradeable | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is this? |
||
| } from "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721ReceiverUpgradeable.sol"; | ||
|
|
||
| import { ProvableNFT } from "./ProvableNFT.sol"; | ||
| import { DirectPaymentsFactory } from "./DirectPaymentsFactory.sol"; | ||
|
|
@@ -48,6 +50,7 @@ contract DirectPaymentsPool is | |
| error NO_BALANCE(); | ||
| error NFTTYPE_CHANGED(); | ||
| error EMPTY_MANAGER(); | ||
| error LENGTH_MISMATCH(); | ||
|
|
||
| bytes32 public constant MANAGER_ROLE = keccak256("MANAGER_ROLE"); | ||
| bytes32 public constant MEMBER_ROLE = keccak256("MEMBER_ROLE"); | ||
|
|
@@ -214,6 +217,22 @@ contract DirectPaymentsPool is | |
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * @dev Adds multiple members to the pool in a single transaction. | ||
| * @param members Array of member addresses to add. | ||
| * @param extraData Array of additional validation data for each member. | ||
| */ | ||
| function addMembers(address[] calldata members, bytes[] calldata extraData) external onlyRole(MANAGER_ROLE) { | ||
| if (members.length != extraData.length) revert LENGTH_MISMATCH(); | ||
|
|
||
| for (uint i = 0; i < members.length; ) { | ||
| _addMember(members[i], extraData[i]); | ||
| unchecked { | ||
| ++i; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why aren't you using the regular for loop format? |
||
| } | ||
| } | ||
| } | ||
|
|
||
| function _grantRole(bytes32 role, address account) internal virtual override { | ||
| if (role == MEMBER_ROLE) { | ||
| registry.addMember(account); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,9 @@ import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils | |
| import { IERC721Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721Upgradeable.sol"; | ||
| import { SafeERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol"; | ||
| import { IERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol"; | ||
| import { IERC721ReceiverUpgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721ReceiverUpgradeable.sol"; | ||
| import { | ||
| IERC721ReceiverUpgradeable | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is this? |
||
| } from "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721ReceiverUpgradeable.sol"; | ||
|
|
||
| import "../GoodCollective/GoodCollectiveSuperApp.sol"; | ||
| import "./UBIPoolFactory.sol"; | ||
|
|
@@ -23,6 +25,7 @@ contract UBIPool is AccessControlUpgradeable, GoodCollectiveSuperApp, UUPSUpgrad | |
| error EMPTY_MANAGER(); | ||
| error MAX_MEMBERS_REACHED(); | ||
| error MAX_PERIOD_CLAIMERS_REACHED(uint256 claimers); | ||
| error LENGTH_MISMATCH(); | ||
|
|
||
| bytes32 public constant MANAGER_ROLE = keccak256("MANAGER_ROLE"); | ||
| bytes32 public constant MEMBER_ROLE = keccak256("MEMBER_ROLE"); | ||
|
|
@@ -189,10 +192,8 @@ contract UBIPool is AccessControlUpgradeable, GoodCollectiveSuperApp, UUPSUpgrad | |
|
|
||
| nextPeriodPool = status.dailyCyclePool; | ||
| nextDailyUbi; | ||
| if ( | ||
| (currentDayInCycle() + 1) >= status.currentCycleLength || shouldStartEarlyCycle | ||
| ) //start of cycle or first time | ||
| { | ||
| if ((currentDayInCycle() + 1) >= status.currentCycleLength || shouldStartEarlyCycle) { | ||
| //start of cycle or first time | ||
| nextPeriodPool = currentBalance / ubiSettings.cycleLengthDays; | ||
| newCycle = true; | ||
| } | ||
|
|
@@ -296,6 +297,22 @@ contract UBIPool is AccessControlUpgradeable, GoodCollectiveSuperApp, UUPSUpgrad | |
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * @dev Adds multiple members to the pool in a single transaction. | ||
| * @param members Array of member addresses to add. | ||
| * @param extraData Array of additional validation data for each member. | ||
| */ | ||
| function addMembers(address[] calldata members, bytes[] calldata extraData) external onlyRole(MANAGER_ROLE) { | ||
| if (members.length != extraData.length) revert LENGTH_MISMATCH(); | ||
|
|
||
| for (uint i = 0; i < members.length; ) { | ||
| addMember(members[i], extraData[i]); | ||
| unchecked { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not use regular for loop format? |
||
| ++i; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function removeMember(address member) external onlyRole(MANAGER_ROLE) { | ||
| _revokeRole(MEMBER_ROLE, member); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ contract UBIPoolFactory is AccessControlUpgradeable, UUPSUpgradeable { | |
| event PoolDetailsChanged(address indexed pool, string ipfs); | ||
| event PoolVerifiedChanged(address indexed pool, bool isVerified); | ||
| event UpdatedImpl(address indexed impl); | ||
| event MemberAdded(address indexed member, address indexed pool); | ||
|
|
||
| struct PoolRegistry { | ||
| string ipfs; | ||
|
|
@@ -172,6 +173,16 @@ contract UBIPoolFactory is AccessControlUpgradeable, UUPSUpgradeable { | |
|
|
||
| function addMember(address account) external onlyPool { | ||
| memberPools[account].push(msg.sender); | ||
| emit MemberAdded(account, msg.sender); | ||
| } | ||
|
|
||
| function addMembers(address[] calldata members) external onlyPool { | ||
| for (uint i = 0; i < members.length; ) { | ||
| addMember(members[i]); | ||
| unchecked { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. regular for loop |
||
| ++i; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function removeMember(address member) external onlyPool { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,236 @@ | ||
| import { deploySuperGoodDollar } from "@gooddollar/goodprotocol"; | ||
| import { loadFixture } from "@nomicfoundation/hardhat-network-helpers"; | ||
| import { deployTestFramework } from "@superfluid-finance/ethereum-contracts/dev-scripts/deploy-test-framework"; | ||
| import "@nomicfoundation/hardhat-toolbox"; | ||
| import { expect } from "chai"; | ||
| import { DirectPaymentsFactory, DirectPaymentsPool, ProvableNFT } from "typechain-types"; | ||
| import { ethers, upgrades } from "hardhat"; | ||
| import { MockContract, deployMockContract } from "ethereum-waffle"; | ||
|
|
||
| type SignerWithAddress = Awaited<ReturnType<typeof ethers.getSigner>>; | ||
|
|
||
| describe("DirectPayments Bulk Members", () => { | ||
| let pool: DirectPaymentsPool; | ||
| let factory: DirectPaymentsFactory; | ||
| let nft: ProvableNFT; | ||
| let signer: SignerWithAddress; | ||
| let signers: SignerWithAddress[]; | ||
| let poolSettings: DirectPaymentsPool.PoolSettingsStruct; | ||
| let poolLimits: DirectPaymentsPool.SafetyLimitsStruct; | ||
| let gdframework: Awaited<ReturnType<typeof deploySuperGoodDollar>>; | ||
| let membersValidator: MockContract; | ||
|
|
||
| before(async () => { | ||
| const { frameworkDeployer } = await deployTestFramework(); | ||
| const sfFramework = await frameworkDeployer.getFramework(); | ||
|
|
||
| signers = await ethers.getSigners(); | ||
|
|
||
| gdframework = await deploySuperGoodDollar(signers[0], sfFramework, [ | ||
| ethers.constants.AddressZero, | ||
| ethers.constants.AddressZero | ||
| ]); | ||
| signer = signers[0]; | ||
| poolSettings = { | ||
| nftType: 1, | ||
| uniquenessValidator: ethers.constants.AddressZero, | ||
| rewardPerEvent: [100, 300], | ||
| validEvents: [1, 2], | ||
| manager: signer.address, | ||
| membersValidator: ethers.constants.AddressZero, | ||
| rewardToken: gdframework.GoodDollar.address, | ||
| allowRewardOverride: false | ||
| }; | ||
|
|
||
| poolLimits = { | ||
| maxMemberPerDay: 300, | ||
| maxMemberPerMonth: 1000, | ||
| maxTotalPerMonth: 3000 | ||
| }; | ||
| }); | ||
|
|
||
| const fixture = async () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dont duplicate code. import from existing tests
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you didnt reduce duplicated code here
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. still didnt address this comment |
||
| const nftFactory = await ethers.getContractFactory("ProvableNFT"); | ||
| nft = (await upgrades.deployProxy(nftFactory, ["nft", "cc"], { kind: "uups" })) as ProvableNFT; | ||
| const helper = await ethers.deployContract("HelperLibrary"); | ||
| const helper2 = await ethers.deployContract("DirectPaymentsLibrary"); | ||
|
|
||
| const Pool = await ethers.getContractFactory("DirectPaymentsPool", { | ||
| libraries: { HelperLibrary: helper.address, DirectPaymentsLibrary: helper2.address } | ||
| }); | ||
| membersValidator = await deployMockContract(signers[0], [ | ||
| "function isMemberValid(address pool,address operator,address member,bytes memory extraData) external returns (bool)" | ||
| ]); | ||
| const poolImpl = await Pool.deploy(await gdframework.GoodDollar.getHost(), ethers.constants.AddressZero); | ||
|
|
||
| factory = (await upgrades.deployProxy( | ||
| await ethers.getContractFactory("DirectPaymentsFactory"), | ||
| [signer.address, poolImpl.address, nft.address, ethers.constants.AddressZero, 0], | ||
| { kind: "uups" } | ||
| )) as DirectPaymentsFactory; | ||
|
|
||
| await nft.grantRole(ethers.constants.HashZero, factory.address); | ||
| // all members are valid by default | ||
| membersValidator.mock["isMemberValid"].returns(true); | ||
|
|
||
| const poolTx = await ( | ||
| await factory.createPool( | ||
| "test-project", | ||
| "ipfs", | ||
| { ...poolSettings, membersValidator: membersValidator.address }, | ||
| poolLimits, | ||
| 0 | ||
| ) | ||
| ).wait(); | ||
| const poolAddress = poolTx.events?.find((_) => _.event === "PoolCreated")?.args?.[0]; | ||
| pool = Pool.attach(poolAddress) as DirectPaymentsPool; | ||
|
|
||
| await gdframework.GoodDollar.mint(pool.address, ethers.constants.WeiPerEther.mul(100000)).then((_: any) => | ||
| _.wait() | ||
| ); | ||
| }; | ||
|
|
||
| beforeEach(async function () { | ||
| await loadFixture(fixture); | ||
| }); | ||
|
|
||
| describe("addMembers - Pool", () => { | ||
| it("should add multiple valid members successfully", async () => { | ||
| const members = [signers[1].address, signers[2].address, signers[3].address]; | ||
| const extraData = ["0x", "0x", "0x"]; | ||
|
|
||
| const tx = await pool.connect(signer).addMembers(members, extraData); | ||
| await expect(tx).not.reverted; | ||
|
|
||
| // Verify all members have MEMBER_ROLE | ||
| for (const member of members) { | ||
| expect(await pool.hasRole(await pool.MEMBER_ROLE(), member)).to.be.true; | ||
| } | ||
|
|
||
| // Verify RoleGranted events emitted | ||
| const receipt = await tx.wait(); | ||
| const roleGrantedEvents = receipt.events?.filter( | ||
| (e) => e.event === "RoleGranted" && e.address === pool.address | ||
| ); | ||
| expect(roleGrantedEvents?.length).to.equal(3); | ||
| }); | ||
|
|
||
| it("should skip duplicate members without reverting", async () => { | ||
| const members = [signers[1].address, signers[2].address, signers[1].address]; // duplicate | ||
| const extraData = ["0x", "0x", "0x"]; | ||
|
|
||
| const tx = await pool.connect(signer).addMembers(members, extraData); | ||
| await expect(tx).not.reverted; | ||
|
|
||
| // Verify unique members were added | ||
| expect(await pool.hasRole(await pool.MEMBER_ROLE(), signers[1].address)).to.be.true; | ||
| expect(await pool.hasRole(await pool.MEMBER_ROLE(), signers[2].address)).to.be.true; | ||
|
|
||
| // Verify only 2 RoleGranted events emitted (duplicate skipped) | ||
| const receipt = await tx.wait(); | ||
| const roleGrantedEvents = receipt.events?.filter( | ||
| (e) => e.event === "RoleGranted" && e.address === pool.address | ||
| ); | ||
| expect(roleGrantedEvents?.length).to.equal(2); | ||
| }); | ||
|
|
||
| it("should skip invalid members when membersValidator rejects them", async () => { | ||
| const members = [signers[1].address, signers[2].address, signers[3].address]; | ||
| const extraData = ["0x", "0x", "0x"]; | ||
|
|
||
| // Mock validator to reject second member | ||
| membersValidator.mock["isMemberValid"].returns(true); | ||
| membersValidator.mock["isMemberValid"] | ||
| .withArgs(pool.address, signer.address, signers[2].address, "0x") | ||
| .returns(false); | ||
|
|
||
| const tx = await pool.connect(signer).addMembers(members, extraData); | ||
| await expect(tx).not.reverted; | ||
|
|
||
| // Verify valid members were added | ||
| expect(await pool.hasRole(await pool.MEMBER_ROLE(), signers[1].address)).to.be.true; | ||
| expect(await pool.hasRole(await pool.MEMBER_ROLE(), signers[3].address)).to.be.true; | ||
|
|
||
| // Verify invalid member was skipped | ||
| expect(await pool.hasRole(await pool.MEMBER_ROLE(), signers[2].address)).to.be.false; | ||
| }); | ||
|
|
||
| it("should revert if members and extraData arrays have different lengths", async () => { | ||
| const members = [signers[1].address, signers[2].address]; | ||
| const extraData = ["0x"]; // mismatched length | ||
|
|
||
| await expect(pool.connect(signer).addMembers(members, extraData)).to.be.revertedWithCustomError( | ||
| pool, | ||
| "LENGTH_MISMATCH" | ||
| ); | ||
| }); | ||
|
|
||
| it("should update factory registry for all added members", async () => { | ||
| const members = [signers[1].address, signers[2].address, signers[3].address]; | ||
| const extraData = ["0x", "0x", "0x"]; | ||
|
|
||
| await pool.connect(signer).addMembers(members, extraData); | ||
|
|
||
| // Verify factory registry updated | ||
| for (const member of members) { | ||
| const memberPools = await factory.memberPools(member, 0); | ||
| expect(memberPools).to.equal(pool.address); | ||
| } | ||
| }); | ||
|
|
||
| it("should emit MemberAdded events from factory for each new member", async () => { | ||
| const members = [signers[1].address, signers[2].address, signers[3].address]; | ||
| const extraData = ["0x", "0x", "0x"]; | ||
|
|
||
| const tx = await pool.connect(signer).addMembers(members, extraData); | ||
| const receipt = await tx.wait(); | ||
|
|
||
| // Verify factory emitted MemberAdded events for each member | ||
| const factoryMemberAddedEvents = receipt.events?.filter( | ||
| (e) => e.event === "MemberAdded" && e.address === factory.address | ||
| ); | ||
| expect(factoryMemberAddedEvents?.length).to.equal(3); | ||
|
|
||
| // Verify event args | ||
| for (let i = 0; i < members.length; i++) { | ||
| const event = factoryMemberAddedEvents?.[i]; | ||
| expect(event?.args?.[0]).to.equal(members[i]); | ||
| expect(event?.args?.[1]).to.equal(pool.address); | ||
| } | ||
| }); | ||
|
|
||
| it("should handle large batch of 100 members", async () => { | ||
| const members = Array(100) | ||
| .fill(0) | ||
| .map((_, i) => ethers.Wallet.createRandom().address); | ||
| const extraData = Array(100).fill("0x"); | ||
|
|
||
| const tx = await pool.connect(signer).addMembers(members, extraData); | ||
| await expect(tx).not.reverted; | ||
|
|
||
| // Verify all members added | ||
| for (const member of members) { | ||
| expect(await pool.hasRole(await pool.MEMBER_ROLE(), member)).to.be.true; | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| describe("Gas Measurement", () => { | ||
| it("should measure gas for different batch sizes", async () => { | ||
| const batchSizes = [10, 50, 100]; | ||
|
|
||
| for (const size of batchSizes) { | ||
| const members = Array(size) | ||
| .fill(0) | ||
| .map((_, i) => ethers.Wallet.createRandom().address); | ||
| const extraData = Array(size).fill("0x"); | ||
|
|
||
| const tx = await pool.connect(signer).addMembers(members, extraData); | ||
| const receipt = await tx.wait(); | ||
|
|
||
| console.log(`Gas used for ${size} members: ${receipt.gasUsed.toString()}`); | ||
| expect(receipt.gasUsed).to.be.lt(30000000); // Should be under block gas limit | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.