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

Update registry interface #40

Merged
merged 3 commits into from
Sep 27, 2023
Merged

Conversation

akshay-ap
Copy link
Contributor

@akshay-ap akshay-ap commented Sep 21, 2023

Fixes #39

Changes in PR:

  • Define values for each module type
  • Update registry's check(...) function as follows:
function check(address module) external view returns (uint64 listedAt, uint64 flaggedAt, uint8 moduleTypes);

moduleTypes would be zero if input address is not listed.

Why is this change needed is explained in #39.
TLDR; Need information from registry on the type of module.

Considerations while accepting this change as a solution:

If a contract is flagged in registry, it will be considered as malicious for all the types it is registered for. e.g., Suppose a contract is added in Registry as a Plugin and Function Handler and later gets flagged because of an issue in its behaviour as a plugin. Now, the check(address) would return flaggedAt > 0 meaning it is flagged a malicious as a whole and not just for its plugin implementation.

Why this approach?
This approach is a simplified model that is easy to follow and implement with low cognitive complexity to understand how flaggedAt works.

listedAt would be the timestamp when module is first added in the registry.

Proposed implementation: 5afe/safe-core-protocol#106

Other alternatives:

a. Allow checking a module per type

In this case interface would look like as follows:

function check(address module, uint8 moduleType) external view returns (uint64 listedAt, uint64 flaggedAt);

Here moduleType must be a power of 2 and function should revert for other values.
In this approach the contract returns values depending on both address of module and moduleType.

  • This provides flexibility to add a module with different types progressively and function would return accurate times when it was listed and flagged.
  • Scenarios supported: A module can be flagged as Plugin and Procotol manager would block it. But, same module can be allowed as Function handler in the Protocol.

b. Return values indicate individual module type, listedAt and flagged Info.

In this case interface would look like as follows:

struct ModuleReturnInfo {
    uint64 listedAt;
    uint64 flaggedAt;
    uint8 moduleType;
}
function check(address module) external view returns (ModuleReturnInfo[] memory moduleRetrunInfoArray);
  • This approach will lead to more gas usage

@akshay-ap akshay-ap added documentation Improvements or additions to documentation enhancement New feature or request labels Sep 21, 2023
@akshay-ap akshay-ap self-assigned this Sep 21, 2023
@akshay-ap akshay-ap marked this pull request as draft September 21, 2023 14:21
@akshay-ap akshay-ap marked this pull request as ready for review September 21, 2023 14:58
@akshay-ap akshay-ap requested a review from rmeissner September 21, 2023 14:59
@rmeissner
Copy link
Member

Personally I prefer alternative solution (a) for the following reasons:

  • It abstracts away implementation details from the registry. I.e. in the current proposed solution the consumer of the registry needs to know how the moduleTypes is encoded (bitwise in the current proposal). While in the alternative solution this is not necessary as the moduleType parameter could be a bytes32 that can be anything.
  • The alternative solution separates the logic how a module is flagged based on module type. This again abstract away internal registry logic from the consumer (e.g. manager)
  • An additional minor point is that the alternative solution is closer to ERC-7484
    • Similar return types

@akshay-ap
Copy link
Contributor Author

akshay-ap commented Sep 26, 2023

Personally I prefer alternative solution (a) for the following reasons:

  • It abstracts away implementation details from the registry. I.e. in the current proposed solution the consumer of the registry needs to know how the moduleTypes is encoded (bitwise in the current proposal). While in the alternative solution this is not necessary as the moduleType parameter could be a bytes32 that can be anything.

  • The alternative solution separates the logic how a module is flagged based on module type. This again abstract away internal registry logic from the consumer (e.g. manager)

  • An additional minor point is that the alternative solution is closer to ERC-7484

    • Similar return types

Updated interface in commit 211266f

@akshay-ap akshay-ap merged commit 19728b3 into main Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update registry interface: Take into consideration module types
3 participants