Skip to content
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

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

Merged

Conversation

leonardotc
Copy link
Contributor

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

  • It changes the interface by adding the missing property as a "complex type".
  • It adds the type since that is the result of the interface
  • It adds assertions to test that scenario

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8848383373

Details

  • 230 of 248 (92.74%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+5.2%) to 78.812%

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%
Totals Coverage Status
Change from base Build 8602216308: 5.2%
Covered Lines: 704
Relevant Lines: 838

💛 - Coveralls

@dasanra dasanra marked this pull request as draft April 26, 2024 14:05
@leonardotc leonardotc force-pushed the fix/protocol-kit/getModulesPaginated branch 2 times, most recently from 9ca8c6f to 2fae63e Compare April 26, 2024 14:32
@leonardotc leonardotc changed the base branch from main to feat/add-eip1193-provider April 26, 2024 14:45
@leonardotc leonardotc force-pushed the fix/protocol-kit/getModulesPaginated branch 2 times, most recently from caaf65c to 4d2bf88 Compare April 26, 2024 14:49
@leonardotc leonardotc force-pushed the fix/protocol-kit/getModulesPaginated branch from 4d2bf88 to ca970c3 Compare April 26, 2024 14:51
@leonardotc leonardotc force-pushed the feat/add-eip1193-provider branch from 46c0856 to 6f23e0b Compare April 29, 2024 07:21
@leonardotc
Copy link
Contributor Author

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];
        }

@leonardotc leonardotc marked this pull request as ready for review May 2, 2024 07:49
@dasanra dasanra requested a review from yagopv May 8, 2024 09:40
@yagopv
Copy link
Member

yagopv commented May 8, 2024

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

@yagopv yagopv merged commit e0aeefd into feat/add-eip1193-provider May 8, 2024
20 checks passed
@yagopv yagopv deleted the fix/protocol-kit/getModulesPaginated branch May 8, 2024 11:30
@github-actions github-actions bot locked and limited conversation to collaborators May 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants