Summary:
Regarding the batch version of _execute, it might be worth adding a comment about the msg.value parameter and how it interacts with the delegate call operation. If a method is looking at msg.value to check for payment and is called by _execute, the method might receive stale information, which could result in an unintended action being enabled. At least on the surface it looks like this is the case :).

I encountered this issue while working with Multicall and it seems like it could be relevant to contracts that implement the full spec such as ERC725X.

Adding a comment about msg.value to the code might be helpful, especially since it is a payable parameter. There are currently comments about the state on the delegate call, but msg.value is worth mentioning as well.

Link to a related issue: Uniswap/v3-periphery#52

I propose the following change to ERC734 (as defined in this doc):

Allow each key to only have a single purpose value. This single purpose would actually represent multiple purposes by convention, using bitmasks.

For example:

• ...001: MANAGEMENT
• ...010: EXECUTION

Thus a key with a purpose of 1, 3, or 5, etc. would be a valid management key. A key with a purpose of 2, 3, 6, or, 7, etc. would be a valid execution key. Thus 1 would be a management key, but not an execution key. 2 would be an execution key, but not a management key. 3 would be both a management and execution key.

The rationale of this technique is that it would give us the full flexibility and power of having multiple purposes per key, minus the overhead of managing an array of purposes in solidity. Bitmasks can be implemented efficiently in solidity.

I'm not clear on whether uint256 or bytes32 would make more sense here. Seems like they would both work: https://ethereum.stackexchange.com/questions/28360/where-is-the-barrier-between-using-uint256-and-bytes32

I think it would also make more sense to think of keys being set rather than added. You could call setKey to either add a key or update an existing key.

The updated interface would be something like :

pragma solidity ^0.4.18;

contract ERC725 {
pragma solidity ^0.4.18;

contract ERC725 {

    event KeySet(bytes32 indexed key, uint256 indexed purpose, uint256 indexed keyType);
    event KeyRemoved(bytes32 indexed key, uint256 indexed purpose, uint256 indexed keyType);
    event ExecutionRequested(uint256 indexed executionId, address indexed to, uint256 indexed value, bytes data);
    event Executed(uint256 indexed executionId, address indexed to, uint256 indexed value, bytes data);
    event Approved(uint256 indexed executionId, bool approved);
    event KeysRequiredChanged(uint256 purpose, uint256 number);

    function getKey(bytes32 _key) public constant returns(uint256 purpose, uint256 keyType, bytes32 key);
    function keyHasPurpose(bytes32 _key, uint256 _purpose) public constant returns (bool exists);
    function setKey(bytes32 _key, uint256 _purpose, uint256 _keyType) public returns (bool success);
    function removeKey(bytes32 _key) public returns (bool success);
    function changeKeysRequired(uint256 purpose, uint256 number) external;
    function getKeysRequired(uint256 purpose) external view returns(uint256);
    function execute(address _to, uint256 _value, bytes _data) public returns (uint256 executionId);
    function approve(uint256 _id, bool _approve) public returns (bool success);
}

Thoughts?

Having a hard time justifying the purpose of this contract. Why build another account abstraction on top of an existing accounts structure?

It would be great to have an implementation of ERC734 in this repo (just like we have an ERC725 implementation here). The current draft of the ERC734 spec is here. Please comment if you're interested in working on this and have any questions.

operationType ordering could be more intuitive:

replacing:

with:

0 for call
1 for delegatecall
2 for staticcall
3 for create
4 for create2

What is this issue about?

After releasing the erc725-smart-contracts package, some external contracts that use our contract as a base run into problems with inheritance.
The error provided was: TypeError: Linearization of inheritance graph impossible.

According to the solidity docs:

Languages that allow multiple inheritances have to deal with several problems. One is the Diamond Problem. Solidity is similar to Python in that it uses “C3 Linearization” to force a specific order in the directed acyclic graph (DAG) of base classes. This results in the desirable property of monotonicity but disallows some inheritance graphs. Especially, the order in which the base classes are given in the is directive is important: You have to list the direct base contracts in the order from “most base-like” to “most derived”.

My understanding of most base to most derive, is opposed to the implementation we currently have, for example:

• Current version at 661cd32: abstract contract ERC725XCore is IERC725X, OwnableUnset
• My Understanding of Most Base to Most Derive: abstract contract ERC725XCore is OwnableUnset, IERC725X
  Even though IERC725X is an interface and OwnableUnset is a module.

Another point regarding using ERC165, it should be inherited at the interface level (like all other libraries do), to match our docs/InterfaceCheatSheet and enforce using it with our contracts.

The aim of this issue is to re-order the inheritance of the contracts to match the docs specification and use the standard way of inheriting used by other contract libraries.

Suggested Solutions

• Re-ordering the inherited contracts from most base to most derive
• Inherit the IERC165 at the interface level

I independently gathered the same list as in identity-proposals, and was delighted to find your repository.

Though I had found the ERC725 Alliance, I didn't find this repository until led from identity-proposals, which begs the question: What of EIP\ERC of the original list?

Maybe you wrote a blog post or something, but I figured other people might swing through and wonder the same thing, and like me, weren't certain if they are abandoned, not worth exploring (or sharing), or simply not your focus.

I have one other in my list: EIP712 - for hashing and signing of typed structured data.

while not specific to identity, I thought it might be related (and probably found it in some article about ID)

thanks again, I'll be spending more time here, and checking out your info... and I added these repos to the DID repo list, which I would be grateful to receive feedback on if you have any ideas..

Thanks!

I would like to nudge a discussion about if there should be a OPERATION_DELEGATECALL type. Delegating the call will grant unknown code access to the identity storage which could be a pretty huge attack vector (at least from my point of view) since you can just change the owner, etc.

Thoughts?

In order to cut the gas costs of deployments of Identity and KeyManager contract, we will introduce forwarder (proxy) contract.

Identity and KeyManager contract will be deployed once and will be sitting on some address to be later used by forwarder for inheritance. User will deploy forwarder contract that will have the address of the real contract hardcoded into its code. Calls will be made on forwarder contract and through delegatecall will be forwarded to the real contract. Forwarder will be designed to be as cheap as possible.

We don't have any test coverage for transferring ether using the ERC725 proxy contract. We should test that it can both receive and send ether.

Everywhere we are using reverting function replace with our function which will check if revert message is correct. This will help us be confident that our test are correct and our contracts are working as expected.

Depends on #17

We introduced a custom initializable for a reason of security where we exposed the initialized modifier and only initializing to always check that these values will not be changed in a specific call and manipulation of the storage when interacting with delegatecall.

But using a custom initializable will conflict with other contracts that use a normal version of initializable.

In our case, we introduced it because of delegatecall, but after rethinking about it, we should not add these getters to make a check because they will give a fake security sense as you can change any state variable, not just initialized or onlyInitializing.

So after re-thinking, we should remove the custom initializable and replace it with the one from OZ Upgradeable. First because of the conflicts that it produces and second because of the fake security sense that it gives and if we insist on these checks, they could be implemented in the final contract to be deployed by reading directly from the storage using assembly.

Upgrade to latest beta of Truffle and solve any problems that come along the way

This was proposed by @Amxx in this PR: #26 on the old code base.

owner might be a multisig, this allows read from/management of any eventual owning contract directly from the proxy. Usefull if the owning contract implements interfaces like ERC1271. That way dapps can call them without having to resolve the proxy ownership, thus making the proxy transparent

Very useful and should be discussed here.

This is especially relevant for the ERC725Account type (see types)

See: https://github.com/OpenZeppelin/openzeppelin-solidity/tree/master/test/helpers

I want to create a (react)app like Origin-Playground, but this time using your latest ERC725 standards. I created a truffle project and install (npm install erc725). Something like the followings:

import "erc725/contracts/ERC725/ERC725Account.sol";
 
contract ClaimHolder is ERC725Account, ERC734KeyManager {
    ...
}

Or in the client directory, I do npm install erc725 and directly use ERC725Account and ERC734KeyManager contracts?

There is no way to know if the transaction reverted when you use a client(Ex: web3) with a proxy identity to execute something.

Why not have something like:

function execute(uint256 _operationType, address _to, uint256 _value, bytes calldata _data)
        external
        onlyOwner
    {
        if (_operationType == OPERATION_CALL) {
            if(!executeCall(_to, _value, _data)) revert();
        } else if (_operationType == OPERATION_CREATE) {
            address newContract = executeCreate(_data);
            emit ContractCreated(newContract);
        } else {
            // We don't want to spend users gas if parametar is wrong
            revert();
        }
    } 

An implementation of smart account with 'shared' logic by verifying user signatures (v/r/s) to execute logic would helpfully extend this standard and instruct a DAO primitive.

Basically, the ERC725X + an extra param in execute() for user signature verification in contract.

This is regarding the ERC734 spec.

This would update the execute function signature as such:

function execute(uint256 _operationType, address _to, uint256 _value, bytes _data) returns (uint256 executionId)

This would make the method more flexible. It would allow contract creation and use of delegatecall. This parameter exists on the current draft of the new ERC725: https://github.com/ERC725Alliance/erc725/blob/master/docs/ERC-725.md#execute

The downside is that it may be unnecessary for most use cases and it will use up another parameter and variable. Any thoughts?

Depends on #17