feat: add native registry integration #101

kopy-kat · 2024-06-14T11:35:10Z

This adds an integration into ERC-7484 natively into the account. Users can use the account as is or with a Registry for higher default security.

livingrockrises · 2024-06-19T15:18:28Z

contracts/Nexus.sol

@@ -306,6 +320,10 @@ contract Nexus is INexus, EIP712, BaseAccount, ExecutionHelper, ModuleManager, U
        return _ACCOUNT_IMPLEMENTATION_ID;
    }

+    function setRegistry(IERC7484 newRegistry, address[] calldata attesters, uint8 threshold) external onlyEntryPointOrSelf {
+        _configureRegistry(newRegistry, attesters, threshold);


at which point SA is made to call

function trustAttesters(
uint8 threshold,
address[] memory attesters // deliberately using memory to allow sorting and uniquifying
)

?

That's an optional step with initcode approach we talked about right with specific factory when deploying the account?

yes, this could be called during the initialization by the factory (eg see bootstrap) or after an account has been created already

livingrockrises · 2024-06-19T15:19:18Z

contracts/base/RegistryAdapter.sol

+        IERC7484 _registry = registry;
+        if (address(_registry) != address(0)) {
+            // this will revert if attestations / threshold are not met
+            _registry.check(module, moduleType);


if we do this everytime through modifier
_checkRegistry

that means, we will always call check() function
and SA should have called trustAttestors before that?

when could these be used?
function checkN(address module, address[] calldata attesters, uint256 threshold) external view;
function checkN(address module, uint256 moduleType, address[] calldata attesters, uint256 threshold) external view;

if we enshrine this current code

yes thats right, the smart account should've set their trusted attesters before

imo it makes more sense to store attesters in the registry since they can then also eg be used by a session key module to check sub-modules on the registry

livingrockrises · 2024-06-19T15:21:21Z

contracts/factory/K1ValidatorFactory.sol

@@ -76,7 +97,7 @@ contract K1ValidatorFactory is Stakeable {

        // Create the validator configuration using the Bootstrap library
        BootstrapConfig memory validator = BootstrapLib.createSingleConfig(K1_VALIDATOR, abi.encodePacked(eoaOwner));
-        bytes memory initData = BOOTSTRAPPER.getInitNexusWithSingleValidatorCalldata(validator);
+        bytes memory initData = BOOTSTRAPPER.getInitNexusWithSingleValidatorCalldata(validator, REGISTRY, attesters, threshold);


this would set the registry and also add the attesters during bootstrap?
and we can pass 0 addresses for registry, array of address 0 for attesters and threshold also 0.

livingrockrises · 2024-06-19T15:23:20Z

contracts/factory/RegistryWhitelistFactory.sol

+    error ZeroAddressNotAllowed();
+
+    IERC7484 public immutable REGISTRY;
+    address[] public attesters;


this would maintain attesters here instead of calling trust attesters from the account?

in this case, the factory has its own attesters but the users account will also call trust attesters if the registry provided is not address(0). so this factory behaves pretty much like the whitelist that was there before but the whitelist is in the registry using the attesters set by the owner of the factory

initData in createAccount() allows attesters and threshold to be provided externally. how does it align with attesters and threshold (dynamic cause of ownable) stored in the contract's storage?

So this specific factory will use the factory-native attesters to query the initial modules and then set trustAttesters with the user provided ones and from then on use this

livingrockrises

review i

Aboudjem · 2024-06-19T17:55:54Z

contracts/base/RegistryAdapter.sol

+abstract contract RegistryAdapter {
+    event ERC7484RegistryConfigured(IERC7484 indexed registry);
+
+    IERC7484 registry;


Suggested change

IERC7484 registry;

IERC7484 public registry;

can we also rename to moduleRegistry and add ERC7484 in natspecs

yeah I havent added natspec yet but happy to add it

we will do it nw

Aboudjem · 2024-06-19T18:03:13Z

contracts/factory/RegistryWhitelistFactory.sol

+    }
+
+    function setThreshold(uint8 newThreshold) external onlyOwner {
+        threshold = newThreshold;


should check current array size

Aboudjem · 2024-06-19T18:04:12Z

contracts/utils/Bootstrap.sol

this is to be removed?

yes. I think we can get our questions answered and get started with decided changes / intervene.

Aboudjem · 2024-06-19T18:05:56Z

contracts/utils/RegistryBootstrap.sol

+
+/// @title Bootstrap
+/// @notice Manages the installation of modules into Nexus smart accounts using delegatecalls.
+contract Bootstrap is ModuleManager {


If we assume this will be used on all factories and part of Nexus, can we merge the RegistryAdapter into ModuleManager and move the state variable to Storage.sol?

different contracts are good for separation of concern.

Bootstrap could be is ModuleManager, RegistryAdapter (keeping registry storage in adapter)

or maybe just moving storage to Storage.sol

livingrockrises · 2024-06-21T09:23:16Z

contracts/utils/RegistryBootstrap.sol

+    /// @param executors The configuration array for executor modules.
+    /// @param hook The configuration for the hook module.
+    /// @param fallbacks The configuration array for fallback handler modules.
+    function initNexus(


is it okay to have two kinds of methods here withRegistry and withoutRegistry
and similarly K1ValidatorFactory and K1ValidatorFactoryWithRegistry

if I use K1ValidatorFactoryWithRegistry with 0 addresses then it would unnecessarily increase gas. we can do comparison benchmark though @Aboudjem

livingrockrises

LGTM. we need to review once again about how storage is managed (the part where RegistryBootstrap is ModuleManager) whereas registry storage is in registry adapter. as we use bootstrap to delegate call to.
@Aboudjem @kopy-kat

livingrockrises

review ii

livingrockrises · 2024-06-21T09:29:30Z

contracts/factory/RegistryWhitelistFactory.sol

+    error ZeroAddressNotAllowed();
+
+    IERC7484 public immutable REGISTRY;
+    address[] public attesters;


initData in createAccount() allows attesters and threshold to be provided externally. how does it align with attesters and threshold (dynamic cause of ownable) stored in the contract's storage?

feat: add native registry integration

8d4b7c2

Aboudjem changed the base branch from main to dev June 19, 2024 11:30

Merge branch 'dev' into feature/native-registry

2302543

Aboudjem requested review from Aboudjem, livingrockrises and filmakarov and removed request for Aboudjem and livingrockrises June 19, 2024 11:33

livingrockrises reviewed Jun 19, 2024

View reviewed changes

Merge branch 'dev' into feature/native-registry

2ce193b

Aboudjem reviewed Jun 19, 2024

View reviewed changes

livingrockrises reviewed Jun 21, 2024

View reviewed changes

livingrockrises changed the base branch from dev to feature/native-registry June 24, 2024 07:11

livingrockrises added 2 commits June 24, 2024 11:20

lint fixes

b75f6fb

refactor

4c867c0

livingrockrises approved these changes Jun 24, 2024

View reviewed changes

livingrockrises merged commit 14cfa4b into bcnmy:feature/native-registry Jun 25, 2024
3 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add native registry integration #101

feat: add native registry integration #101

kopy-kat commented Jun 14, 2024

livingrockrises Jun 19, 2024

kopy-kat Jun 20, 2024

livingrockrises Jun 19, 2024

livingrockrises Jun 19, 2024

kopy-kat Jun 20, 2024

kopy-kat Jun 20, 2024

livingrockrises Jun 19, 2024

kopy-kat Jun 20, 2024

livingrockrises Jun 19, 2024

kopy-kat Jun 20, 2024

livingrockrises Jun 21, 2024

kopy-kat Jun 21, 2024

livingrockrises left a comment

Aboudjem Jun 19, 2024

livingrockrises Jun 19, 2024

kopy-kat Jun 20, 2024

livingrockrises Jun 21, 2024

Aboudjem Jun 19, 2024

Aboudjem Jun 19, 2024

livingrockrises Jun 19, 2024

Aboudjem Jun 19, 2024

livingrockrises Jun 19, 2024

livingrockrises Jun 21, 2024

livingrockrises left a comment

livingrockrises left a comment

livingrockrises Jun 21, 2024

feat: add native registry integration #101

feat: add native registry integration #101

Conversation

kopy-kat commented Jun 14, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

livingrockrises 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

livingrockrises left a comment

Choose a reason for hiding this comment

livingrockrises left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment