dapphub / ds-auth Goto Github PK

_authority is available as a public accessor, yet it is _ prefixed. This seems contradictory - why not expose without the _?

Hi, we recently have conducted a systematic study about Solidity event usage, evolution, and impact, and we are attempting to build a tool to improve the practice of Solidity event use based on our findings. We have tried our prototype tool on some of the most popular GitHub Solidity repositories, and for your repository, we find a potential optimization of gas consumption arisen from event use.

The point is that when we use emit operation to store the value of a certain variable, local memory type variable would be preferable to global storage type (state) variable if they hold the same value. The reason is that an extra SLOAD operation would be needed to access the variable if it is storage type, and the SLOAD operation costs 800 gas.

For your repository, we find that the following event use can be improved:

• auth.sol
  function name: setOwner
  event name:  LogSetOwner
  variable:    owner->owner_

    function setOwner(address owner_)
        public
        auth
    {
        owner = owner_;
        emit LogSetOwner(owner);
    }

Do you find our results useful? Your reply and invaluable suggestions would be greatly appreciated, and are vital for improving our tool. Thanks a lot for your time!

Hi,

The function DSAuth.setOwner emits an event LogSetOwner, which is not documented in the README doc.

A potential fix could be adding "Emits a LogSetOwner event" to the README doc.

Could you please check it?

Thanks.

constant or not is part of the function signature for override purposes. constant has no real effect except for metadata yet, but that may change

Hey, I just noticed some discrepencies between the documentation of DSAuth and the implementation of DSAuth:

1. The documentation states that there would be a modifier authorized, but that doesn't seem to exist in the implementation.
2. The documentation suggests that modifer authorized would would accept a single argument of type bytes4, but the examples show an argument of type string.

My suggestion is to overload authorized to accept types of both string and bytes4:

// This can be used with static strings.
modifier authorized (string _action) 
{
    assert(isAuthorized(msg.sender, bytes4(sha3(_action))));
    _;
}

// This should be used with dynamic strings.
// The caller will have to manually generate a signature, e.g., `bytes4(sha3(stringVar))`                                                            
modifier authorized (bytes4 _sig) 
{
    assert(isAuthorized(msg.sender, _sig));
    _;
}

https://github.com/dapphub/ds-auth/blob/master/contracts/DSAuth.sol#L67

Inside the contract lies the following code :

if (authority == DSAuthority(0))

Where authority is set ? And what DSAuthority(0) does ?

the auth mode is an enum because I anticipated more auth modes, but that didn't happen. canCall is a nice general ACL scheme.

An authority can fully simulate owner mode by returning true for canCall(authority, target, bytes4(sha3("updateAuthority(bytes4,uint8)"))

brainstorming DSAuthorized changes:

• when in "auth" mode, falls back to msg.sender check
• enableAuthLookup/disableAuthLookup, internally auth_lookup_enabled instead of enum
• releaseToAuthority - direct to auth mode
• releaseToOwner - direct to owner mode