safe-global / safe-core-sdk

The Safe{Core} SDK allows builders to add account abstraction functionality into their apps.
https://docs.safe.global/sdk/overview
MIT License
243 stars 178 forks source link

fix(protocol kit): Get modules paginated incorrect interface #787

Closed leonardotc closed 1 month ago

leonardotc commented 2 months ago

What it solves

The method getModulesPaginated should return a tuple containing both the modules as well as the next address of the page.

How this PR fixes it

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 8848383373

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/relay-kit/src/packs/safe-4337/utils.ts 8 10 80.0%
packages/relay-kit/src/packs/gelato/GelatoRelayPack.ts 6 11 54.55%
packages/relay-kit/src/packs/safe-4337/Safe4337Pack.ts 146 157 92.99%
<!-- Total: 230 248 92.74% -->
Totals Coverage Status
Change from base Build 8602216308: 5.2%
Covered Lines: 704
Relevant Lines: 838

💛 - Coveralls
leonardotc commented 2 months ago

This was added to the 1.4.1 of the contract (https://github.com/safe-global/safe-smart-account/blob/v1.4.1/contracts/base/ModuleManager.sol:

        /**
          Because of the argument validation, we can assume that the loop will always iterate over the valid module list values
          and the `next` variable will either be an enabled module or a sentinel address (signalling the end). 

          If we haven't reached the end inside the loop, we need to set the next pointer to the last element of the modules array
          because the `next` variable (which is a module by itself) acting as a pointer to the start of the next page is neither 
          included to the current page, nor will it be included in the next one if you pass it as a start.
        */
        if (next != SENTINEL_MODULES) {
            next = array[moduleCount - 1];
        }
yagopv commented 1 month ago

Beautiful @leonardotc thanks!. I think you need to pull from the target branch for fixing the failing tests