Feat/v3

[PatrickAlphaC]

work in progress to solve: #39

TODO:

• Get final list of owner addresses and chains to deploy new contracts to

TODO (after deployment):

• Update references to on-chain contracts after deployment in legal docs

[Robert-MacWha]

Is there a reason for the libs to be copied instead of references for legacy? Might have just been a mistake when cloning?

[PatrickAlphaC]

Is there a reason for the libs to be copied instead of references for legacy? Might have just been a mistake when cloning?

Fixed!

[Robert-MacWha]

The constructor is clever. If we want to retain the same address, we can just include all the adopters and the invalid ones will be skipped. And if there was no deployed registry it'll skip over the entire block.

Would it be better to exclude the chain validation logic from the registry? Right now the registry serves dual responsibilities, and opens the door to needing a new registry (or at least deprecating the current one) if the interface for agreement verification changes too much.

I would tentatively suggest:

contract SafeHarborRegistry is IRegistry { // Remove Ownable since no onlyOwner methods 
    error SafeHarborRegistry__NoAgreement();
    error SafeHarborRegistry__ZeroAddress();
    
    mapping(address entity => address details) private agreements;
    
    event SafeHarborAdoption(address indexed adopter, address agreementAddress);
    event LegacyDataMigrated(address indexed legacyRegistry, uint256 migratedCount);
    
    constructor(address _legacyRegistry, address[] memory _adopters)
    function adoptSafeHarbor(address _agreementAddress) external
    function version() external pure returns (string memory)
    function getAgreement(address _adopter) external view returns (address)
}

contract AgreementFactory {
    address private constant REGISTRY_ADDRESS;
    address private constant CHAIN_VALIDATOR_ADDRESS;
    function create(AgreementDetails memory details, address owner) external returns (address agreementAddress);
}

Or, if we want the chain validator to be variable, either using the AgreementFactory as the proxy or by letting the owners of an Agreement change their CHAIN_VALIDATOR_ADDRESS. Unless there's a good reason for mutable validator addresses I'd make them constant, otherwise would vote to move it to the factory.

[PatrickAlphaC]

exclude the chain validation logic from the registry?

I'm not sure I follow, right now the registry has a function that allows the registry owner to change the chain validation logic contract:

 function setChainValidator(address _newChainValidator) external onlyOwner {
        if (_newChainValidator == address(0)) {
            revert SafeHarborRegistry__ZeroAddress();
        }
        emit ChainValidatorSet(_newChainValidator);
        chainValidator = IChainValidator(_newChainValidator);
    }

We created ChainValidator.sol to do the chain validation logic.

[PatrickAlphaC]

I think the final thing for us to do, would be the following.

In our HelperConfig.s.sol we have different configs for each chain:

function getMainnetConfig() public pure returns (NetworkConfig memory) {
        address[] memory adopters = new address[](5);
        adopters[0] = RHEO;
        adopters[1] = OPS_COVEFI_ETH;
        adopters[2] = AAVE;
        adopters[3] = IDLE_FINANCE;
        adopters[4] = ENS;
        return NetworkConfig({
            owner: SEAL_MAINNET_OWNER,
            legacyRegistry: DEFAULT_LEGACY_REGISTRY,
            createx: CREATEX_ADDRESS,
            adopters: adopters
        });
    }

I'd need someone to make a PR to go through each chain and add the correct config information. I think it might be easy enough to set the owner to be SEAL_MAINNET_OWNER. What do you think?

[Robert-MacWha]

exclude the chain validation logic from the registry?

I'm not sure I follow, right now the registry has a function that allows the registry owner to change the chain validation logic contract:

 function setChainValidator(address _newChainValidator) external onlyOwner {
        if (_newChainValidator == address(0)) {
            revert SafeHarborRegistry__ZeroAddress();
        }
        emit ChainValidatorSet(_newChainValidator);
        chainValidator = IChainValidator(_newChainValidator);
    }

We created ChainValidator.sol to do the chain validation logic.

Was suggesting we could move that proxy logic into a different contract so the registry contract is single-responsibility and isn't ownable

Trying to minimize the chance we ever need to deploy a new registry.

[DicksonWu654]

exclude the chain validation logic from the registry?

I'm not sure I follow, right now the registry has a function that allows the registry owner to change the chain validation logic contract:

 function setChainValidator(address _newChainValidator) external onlyOwner {
        if (_newChainValidator == address(0)) {
            revert SafeHarborRegistry__ZeroAddress();
        }
        emit ChainValidatorSet(_newChainValidator);
        chainValidator = IChainValidator(_newChainValidator);
    }

We created ChainValidator.sol to do the chain validation logic.

Was suggesting we could move that proxy logic into a different contract so the registry contract is single-responsibility and isn't ownable

Trying to minimize the chance we ever need to deploy a new registry.

I agree. Imo right now the Registry is just middle manning the isValidChain function... But we can go direct and just have the ChainValidator inside of the agreements instead. That would simplify the Registry even further to basically be a mapping lol + remove the ownableness of it

I'll include that in my PR

[DicksonWu654]

exclude the chain validation logic from the registry?

I'm not sure I follow, right now the registry has a function that allows the registry owner to change the chain validation logic contract:

 function setChainValidator(address _newChainValidator) external onlyOwner {
        if (_newChainValidator == address(0)) {
            revert SafeHarborRegistry__ZeroAddress();
        }
        emit ChainValidatorSet(_newChainValidator);
        chainValidator = IChainValidator(_newChainValidator);
    }

We created ChainValidator.sol to do the chain validation logic.

Was suggesting we could move that proxy logic into a different contract so the registry contract is single-responsibility and isn't ownable
Trying to minimize the chance we ever need to deploy a new registry.

I agree. Imo right now the Registry is just middle manning the isValidChain function... But we can go direct and just have the ChainValidator inside of the agreements instead. That would simplify the Registry even further to basically be a mapping lol + remove the ownableness of it

I'll include that in my PR

I think the only security consideration here is: If we somehow fuck up the ownership of the ChainValidator, we are fucked and need the protocols to deploy a new one and register the new one instead. If you have the option to change it, you can have a backup inside of the ownership of the registry to save it.... But it's likely they're the same owner? Also prob unlikely scenario?

[DicksonWu654]

Yooo btw your construcutor is hella clever. Smart as hell

[Robert-MacWha]

Wanted to check when this was created. I noticed it's not running - should it be, or if not can we remove it?

[DicksonWu654]

Merged now - Thank you so so so much Patrick & Cyfrin ❤️

[PatrickAlphaC]

Beautiful!