diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol
index 3db31b11c..ac3fe35cd 100644
--- a/contracts/base/ModuleManager.sol
+++ b/contracts/base/ModuleManager.sol
@@ -107,23 +107,37 @@ 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) {
- array[moduleCount] = currentModule;
- currentModule = modules[currentModule];
+ next = modules[start];
+ while (next != address(0) && next != SENTINEL_MODULES && moduleCount < pageSize) {
+ array[moduleCount] = next;
+ next = modules[next];
moduleCount++;
}
- next = currentModule;
+
+ // Because of the argument validation we can assume that
+ // the `currentModule` will always be either a module address
+ // or sentinel address (aka the end). If we haven't reached the end
+ // inside the loop, we need to set the next pointer to the last element
+ // because it skipped over to the next module which is neither included
+ // in the current page nor won't be included in the next one
+ // if you pass it as a start.
+ if (next != SENTINEL_MODULES) {
+ next = array[moduleCount - 1];
+ }
// 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
index c856d1070..4611d18db 100644
--- 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 set up`
diff --git a/test/core/GnosisSafe.ModuleManager.spec.ts b/test/core/GnosisSafe.ModuleManager.spec.ts
index 1cf700a48..e480d0ddd 100644
--- a/test/core/GnosisSafe.ModuleManager.spec.ts
+++ b/test/core/GnosisSafe.ModuleManager.spec.ts
@@ -9,7 +9,7 @@ import { AddressOne } from "../../src/utils/constants";
describe("ModuleManager", async () => {
- const [user1, user2] = waffle.provider.getWallets();
+ const [user1, user2, user3] = waffle.provider.getWallets();
const setupTests = deployments.createFixture(async ({ deployments }) => {
await deployments.fixture();
@@ -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])
@@ -244,4 +244,52 @@ describe("ModuleManager", async () => {
).to.be.deep.eq([false, "0x08c379a000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000013536f6d652072616e646f6d206d65737361676500000000000000000000000000"])
})
})
+
+ 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.revertedWith("GS106")
+ })
+
+ 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(
+ executeContractCallWithSigners(safe, safe, "enableModule", [user1.address], [user1])
+ ).to.emit(safe, "EnabledModule").withArgs(user1.address)
+
+ await expect(
+ executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1])
+ ).to.emit(safe, "EnabledModule").withArgs(user2.address)
+
+ await expect(
+ executeContractCallWithSigners(safe, safe, "enableModule", [user3.address], [user1])
+ ).to.emit(safe, "EnabledModule").withArgs(user3.address)
+
+ 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], AddressOne])
+ })
+
+ it('returns an empty array and end pointer for a safe with no modules', async () => {
+ const { safe } = await setupTests()
+ expect(await safe.getModulesPaginated(AddressOne, 10)).to.be.deep.equal([[], AddressOne])
+ })
+ })
})
\ No newline at end of file
diff --git a/test/core/GnosisSafe.Setup.spec.ts b/test/core/GnosisSafe.Setup.spec.ts
index dc3de7d1d..be62bc7f7 100644
--- a/test/core/GnosisSafe.Setup.spec.ts
+++ b/test/core/GnosisSafe.Setup.spec.ts
@@ -29,10 +29,15 @@ describe("GnosisSafe", async () => {
await expect(
await singleton.getThreshold()
).to.be.deep.eq(BigNumber.from(1))
- await expect(
- await singleton.getModulesPaginated(AddressOne, 10)
- ).to.be.deep.eq([[], AddressZero])
+ // Because setup wasn't called on the singleton it breaks the assumption made
+ // within `getModulesPaginated` method that the linked list will be always correctly
+ // initialized with 0x1 as a starting element and 0x1 as the end
+ // But because `setupModules` wasn't called, it is empty.
+ await expect(
+ singleton.getModulesPaginated(AddressOne, 10)
+ ).to.be.reverted
+
// "Should not be able to retrieve owners (currently the contract will run in an endless loop when not initialized)"
await expect(
singleton.getOwners()