5afe / safe-core-protocol-specs

Safe{Core} Protocol is an open, modular framework to make smart accounts secure, portable, and composable.
GNU General Public License v3.0
67 stars 13 forks source link

Update registry interface #40

Closed akshay-ap closed 11 months ago

akshay-ap commented 11 months ago

Fixes #39

Changes in PR:

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: https://github.com/safe-global/safe-core-protocol/pull/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.

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);
rmeissner commented 11 months ago

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

akshay-ap commented 11 months ago

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 https://github.com/safe-global/safe-core-protocol-specs/pull/40/commits/211266f35999d6a47f4333534dab566d4f89f7a5