1.4.0: Fix `getModulesPaginated` #470

mmv08 · 2022-12-24T22:44:47Z

This PR fixes #461 by checking if we reached the end of the modules list and conditionally returning either sentinel address signaling the end or the last element of the module list

Also added some test cases and validations for passed arguments

coveralls · 2022-12-24T22:55:53Z

Pull Request Test Coverage Report for Build 3912478679

8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.03%) to 98.511%

Totals
Change from base Build 3904293007:	0.03%
Covered Lines:	311
Relevant Lines:	312

💛 - Coveralls

rmeissner

Looking at this more we need to define a little bit more what is expected here.

E.g.

What should happen if an invalid start is passed? (We also don'T have tests here)
What should be returned at the end of the page? (0 vs 1 vs ?)
- What should be returned for an empty page.

With the current solution we break even the flow for working use cases (e.g. we change what is returned of all entries fit into a single page), which I would avoid

contracts/base/ModuleManager.sol

rmeissner · 2022-12-25T09:30:42Z

test/core/GnosisSafe.ModuleManager.spec.ts

@@ -62,7 +62,7 @@ describe("ModuleManager", async () => {

            await expect(
                await safe.getModulesPaginated(AddressOne, 10)
-            ).to.be.deep.equal([[user2.address], AddressOne])


This tests should not have to be adjusted. I would stick to the defined behavior for this case, meaning at the end of the page AddressOne is returned as next (signaling the end).

I would stick to the defined behavior for this case, meaning at the end of the page AddressOne is returned as next (signaling the end).

It seems like this would require adjustments in the module manager contract, because unlike OwnerManager we don't have the last module pointing to a sentinel address. Do you think it's worth it?

we don't have the last module pointing to a sentinel address.

That should always be the case. This is required else a lot of logic will break

Do we want to do something like this in getModulesPaginated method then:

if (next == address(0)) next = SENTINEL_ADDRESS;

why? I do not understand why this would be required. The linked list always starts with the sentinel and ends on the sentinel:

Empty list: 0x1 -> 0x1

List with one module: 0x1 -> 0xbaddad -> 0x1

List with n modules: 0x1 -> 0xbaddad -> 0x.... -> 0x1

If next is 0x0 then something is wrong

Ah, I was confused how this was possible until I saw that setupModules is called during the setup. Will investigate why this is not the case

rmeissner · 2022-12-25T14:31:37Z

contracts/base/ModuleManager.sol

@@ -123,7 +123,7 @@ contract ModuleManager is SelfAuthorized, Executor {
            currentModule = modules[currentModule];
            moduleCount++;
        }
-        next = currentModule;
+        next = array[moduleCount == 0 ? 0 : moduleCount - 1];


array[0] (for the case that moduleCound == 0) would be undefined, right? In the best case it is 0 in the worst case it some random data (should normally not happen).

I still believe we should first clearly define the expected behavior for this method (and maybe even for the .

start has to be a module else the method should revert

pageSize needs to be greater 0, else the method should revert

If all entries fit into a single page, then next should be Address One (as this is the current behavior)

If another page is present then next should be the last element of the returned array (<- this was bugged before)

array[0] (for the case that moduleCound == 0) would be undefined, right?

It's zero address I think

yeah it should be zero (at least with the current solidity management), but not sure if there is a guarantee for it.

if pageSize is 0 this should revert as array access should do bounds checks.

mmv08 · 2022-12-28T21:33:17Z

@rmeissner a side question:

    function setupModules(address to, bytes memory data) internal {
        require(modules[SENTINEL_MODULES] == address(0), "GS100");
        modules[SENTINEL_MODULES] = SENTINEL_MODULES;
        if (to != address(0))
            // Setup has to complete successfully or transaction fails.
            require(execute(to, 0, data, Enum.Operation.DelegateCall, gasleft()), "GS000");
    }

Should we ensure here that to is a contract? or perhaps in the GnosisSafe setup? Might be valuable for highly modularly customised safes

mmv08 · 2022-12-30T15:35:07Z

@rmeissner I gave it another go, please check it. I still had to adjust some test though.

mmv08 · 2022-12-30T15:36:29Z

contracts/base/ModuleManager.sol

        // 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) {


I changed 0x0 to 0 because within the file this was the only occurence of 0x0 and 0 was used everywhere else

…obal/safe-contracts into bounty_module_pagination

mmv08 · 2023-01-10T09:03:27Z

contracts/base/ModuleManager.sol

    /// @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");


maybe it's worth noting that removing the pageSize check passes all the test except it doesn't revert when passed pageSize === 0 and instead returns an empty array

rmeissner

Besides adding a test case for a Safe that has no modules it looks good

rmeissner · 2023-01-10T09:17:11Z

test/core/GnosisSafe.ModuleManager.spec.ts

@@ -244,4 +244,47 @@ describe("ModuleManager", async () => {
            ).to.be.deep.eq([false, "0x08c379a000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000013536f6d652072616e646f6d206d65737361676500000000000000000000000000"])
        })
    })
+
+    describe("getModulesPaginated", async () => {


Do we have a test for the case that this method is called when no modules are enabled?

rmeissner · 2023-01-10T09:20:00Z

contracts/base/ModuleManager.sol

    /// @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];


Theoretically we should be able to use next instead of currentModule (merge these too), but not sure if that would make it really better

rmeissner · 2023-01-10T11:48:42Z

Should we ensure here that to is a contract? or perhaps in the GnosisSafe setup? Might be valuable for highly modularly customised safes

Didn't see this comment. I like the addition, but would do it in a separate PR.

…nty_module_pagination

rmeissner added 2 commits November 7, 2022 14:45

Add test for module pagination

743af7f

Add test for module pagination

78dadea

mmv08 force-pushed the bounty_module_pagination branch 2 times, most recently from 68ab14a to a5418d4 Compare December 24, 2022 22:53

mmv08 requested a review from a team December 24, 2022 22:54

rmeissner requested changes Dec 25, 2022

View reviewed changes

mmv08 force-pushed the bounty_module_pagination branch 3 times, most recently from 9b5ecde to 1c1ef96 Compare December 25, 2022 11:50

rmeissner reviewed Dec 25, 2022

View reviewed changes

mmv08 force-pushed the bounty_module_pagination branch 2 times, most recently from 93b8f85 to cb3e0aa Compare December 30, 2022 15:29

mmv08 commented Dec 30, 2022

View reviewed changes

mmv08 requested a review from rmeissner December 30, 2022 15:38

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

32c86d9

…obal/safe-contracts into bounty_module_pagination

mmv08 force-pushed the bounty_module_pagination branch from cb3e0aa to 32c86d9 Compare December 30, 2022 15:39

mmv08 commented Jan 10, 2023

View reviewed changes

rmeissner approved these changes Jan 10, 2023

View reviewed changes

Merge branch 'main' of github.com:safe-global/safe-contracts into bou…

069992c

…nty_module_pagination

mmv08 force-pushed the bounty_module_pagination branch from ed1faee to 069992c Compare January 13, 2023 15:07

mmv08 mentioned this pull request Jan 13, 2023

require the to address to be a contract in setupModules #483

Closed

mmv08 merged commit 7126451 into main Jan 13, 2023

mmv08 deleted the bounty_module_pagination branch January 13, 2023 15:14

github-actions bot locked and limited conversation to collaborators Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1.4.0: Fix `getModulesPaginated` #470

1.4.0: Fix `getModulesPaginated` #470

mmv08 commented Dec 24, 2022 •

edited

Loading

coveralls commented Dec 24, 2022 •

edited

Loading

rmeissner left a comment

rmeissner Dec 25, 2022 •

edited

Loading

mmv08 Dec 25, 2022

rmeissner Dec 25, 2022 •

edited

Loading

mmv08 Dec 25, 2022

rmeissner Dec 28, 2022 •

edited

Loading

mmv08 Dec 28, 2022

rmeissner Dec 25, 2022

mmv08 Dec 25, 2022

rmeissner Dec 28, 2022

mmv08 commented Dec 28, 2022 •

edited

Loading

mmv08 commented Dec 30, 2022

mmv08 Dec 30, 2022

mmv08 Jan 10, 2023

rmeissner left a comment

rmeissner Jan 10, 2023

rmeissner Jan 10, 2023

mmv08 Jan 13, 2023

rmeissner commented Jan 10, 2023

1.4.0: Fix getModulesPaginated #470

1.4.0: Fix getModulesPaginated #470

Conversation

mmv08 commented Dec 24, 2022 • edited Loading

coveralls commented Dec 24, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3912478679

💛 - Coveralls

rmeissner left a comment

Choose a reason for hiding this comment

rmeissner Dec 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmeissner Dec 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmeissner Dec 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmv08 commented Dec 28, 2022 • edited Loading

mmv08 commented Dec 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmeissner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmeissner commented Jan 10, 2023

1.4.0: Fix `getModulesPaginated` #470

1.4.0: Fix `getModulesPaginated` #470

mmv08 commented Dec 24, 2022 •

edited

Loading

coveralls commented Dec 24, 2022 •

edited

Loading

rmeissner Dec 25, 2022 •

edited

Loading

rmeissner Dec 25, 2022 •

edited

Loading

rmeissner Dec 28, 2022 •

edited

Loading

mmv08 commented Dec 28, 2022 •

edited

Loading