Merge branch 'bounty_module_pagination' of https://github.com/safe-gl…

…obal/safe-contracts into bounty_module_pagination
safe-global · Dec 30, 2022 · 93b8f85 · 93b8f85
2 parents 78dadea + 743af7f
commit 93b8f85
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 14 deletions.
diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol
@@ -107,24 +107,27 @@ contract ModuleManager is SelfAuthorized, Executor {
     }
 
     /// @dev Returns array of modules.
-    /// @param start Start of the page.
-    /// @param pageSize Maximum number of modules that should be returned.
+    ///      If all entries fit into a single page, the next pointer will be 0x1.
+    ///      If another page is present, next will be the last element of the returned array.
+    /// @param start Start of the page. Has to be a module or start pointer (0x1 address)
+    /// @param pageSize Maximum number of modules that should be returned. Has to be > 0
     /// @return array Array of modules.
     /// @return next Start of the next page.
     function getModulesPaginated(address start, uint256 pageSize) external view returns (address[] memory array, address next) {
+        require(start == SENTINEL_MODULES || isModuleEnabled(start), "GS105");
+        require(pageSize > 0, "GS106");
         // Init array with max page size
         array = new address[](pageSize);
 
         // Populate return array
         uint256 moduleCount = 0;
         address currentModule = modules[start];
-
-        while (currentModule != address(0x0) && currentModule != SENTINEL_MODULES && moduleCount < pageSize) {
+        while (currentModule != address(0) && currentModule != SENTINEL_MODULES && moduleCount < pageSize) {
             array[moduleCount] = currentModule;
-            next = currentModule;
             currentModule = modules[currentModule];
             moduleCount++;
         }
+        next = currentModule;
         // Set correct size of returned array
         // solhint-disable-next-line no-inline-assembly
         assembly {

diff --git a/docs/error_codes.md b/docs/error_codes.md
@@ -29,6 +29,8 @@
 - `GS102`: `Module has already been added`
 - `GS103`: `Invalid prevModule, module pair provided`
 - `GS104`: `Method can only be called from an enabled module`
+- `GS105`: `Invalid starting point for fetching paginated modules`
+- `GS106`: `Invalid page size for fetching paginated modules`
 
 ### Owner management related
 - `GS200`: `Owners have already been setup`

diff --git a/test/core/GnosisSafe.ModuleManager.spec.ts b/test/core/GnosisSafe.ModuleManager.spec.ts
@@ -50,7 +50,7 @@ describe("ModuleManager", async () => {
             ).to.revertedWith("GS013")
         })
 
-        it('emits event for new module', async () => {
+        it('emits event for a new module', async () => {
             const { safe } = await setupTests()
             await expect(
                 executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])
@@ -246,6 +246,20 @@ describe("ModuleManager", async () => {
     })
 
     describe("getModulesPaginated", async () => {
+        it('requires page size to be greater than 0', async () => {
+            const { safe } = await setupTests()
+            await expect(safe.getModulesPaginated(AddressOne, 0)).to.be.reverted
+        })
+
+        it('requires start to be a module or start pointer', async () => {
+            const { safe } = await setupTests()
+
+            await expect(safe.getModulesPaginated(AddressZero, 1)).to.be.reverted
+            await executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user1])
+            expect(await safe.getModulesPaginated(user1.address, 1)).to.be.deep.equal([[], AddressOne])
+            await expect(safe.getModulesPaginated(user2.address, 1)).to.be.revertedWith("GS105")
+        })
+
         it('Returns all modules over multiple pages', async () => {
             const { safe } = await setupTests()
             await expect(
@@ -263,16 +277,14 @@ describe("ModuleManager", async () => {
             await expect(await safe.isModuleEnabled(user1.address)).to.be.true
             await expect(await safe.isModuleEnabled(user2.address)).to.be.true
             await expect(await safe.isModuleEnabled(user3.address)).to.be.true
-
-
+            /*
+            This will pass the test which is not correct
+            await expect(await safe.getModulesPaginated(AddressOne, 1)).to.be.deep.equal([[user3.address], user2.address])
+            await expect(await safe.getModulesPaginated(user2.address, 1)).to.be.deep.equal([[user1.address], AddressOne])
+            */
             await expect(await safe.getModulesPaginated(AddressOne, 1)).to.be.deep.equal([[user3.address], user3.address])
             await expect(await safe.getModulesPaginated(user3.address, 1)).to.be.deep.equal([[user2.address], user2.address])
-            await expect(await safe.getModulesPaginated(user2.address, 1)).to.be.deep.equal([[user1.address], user1.address])
-            await expect(await safe.getModulesPaginated(user1.address, 1)).to.be.deep.equal([[], AddressZero])
-
-            await expect(await safe.getModulesPaginated(AddressOne, 2)).to.be.deep.equal([[user3.address, user2.address], user2.address])
-
-            await expect(await safe.getModulesPaginated(AddressOne, 3)).to.be.deep.equal([[user3.address, user2.address, user1.address], user1.address])
+            await expect(await safe.getModulesPaginated(user2.address, 1)).to.be.deep.equal([[user1.address], AddressOne])
         })
     })
 })