New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Impossible to use modifiers defined in libraries #2467

Open
frangio opened this Issue Jun 26, 2017 · 13 comments

Comments

8 participants
@frangio
Contributor

frangio commented Jun 26, 2017

Modifier invocations only work when the modifier is an identifier. A modifier m defined in library L can only be referred to as the member access L.m. There's no way to use it.

@axic

This comment has been minimized.

Member

axic commented Aug 8, 2017

Would a modifier placed in a library act as an internal library function and thus be inlined or would be called remotely via delegatecall?

@frangio

This comment has been minimized.

Contributor

frangio commented Aug 9, 2017

I was thinking of them as internal.

@dr-orlovsky

This comment has been minimized.

dr-orlovsky commented Sep 29, 2017

Modifiers in libraries are really useful thing. I was working on the state machine library, and function modifiers are good companions to it allowing aspect-oriented applications.

library StateMachine {
    enum State {
        // Since `destroyself()` zeroes values of all variables, we need the first state (corresponding to zero)
        // to indicate that contract had being destroyed
        Destroyed,
        Offline,
        InsufficientStake,
        UnderPenalty,
        Idle,
        Computing
    }

    struct StateMachine {
        bool initialized;
        State currentState;
        mapping(uint8 => State[]) transitionTable;
    }

    modifier transitionToState(
        StateMachine storage _machine,
        State _newState
    ) {
        // Should not happen
        assert(_machine.currentState == State.Destroyed);

        // Checking if the state transition is allowed
        State[] storage allowedStates = _machine.transitionTable[uint8(_machine.currentState)];
        for (uint no = 0; no < allowedStates.length; no++) {
            if (allowedStates[no] == _newState) {
                _;
                _machine.currentState = _newState;
                return;
            }
        }

        revert();
    }

    modifier transitionThroughState(
        StateMachine storage _machine,
        State _transitionState
    ) {
        // Should not happen
        assert(_machine.currentState == State.Destroyed);

        // Checking if the state transitions are allowed

        bool firstTransitionAllowed = false;
        State[] storage allowedStates = _machine.transitionTable[uint8(_machine.currentState)];
        for (uint no = 0; no < allowedStates.length; no++) {
            if (allowedStates[no] == _transitionState) {
                firstTransitionAllowed = true;
                break;
            }
        }
        require(firstTransitionAllowed == true);

        bool secondTransitionAllowed = false;
        allowedStates = _machine.transitionTable[uint8(_transitionState)];
        for (no = 0; no < allowedStates.length; no++) {
            if (allowedStates[no] == _machine.currentState) {
                secondTransitionAllowed = true;
                break;
            }
        }
        require(secondTransitionAllowed == true);

        State initialState = _machine.currentState;
        _machine.currentState = _transitionState;
        _;
        _machine.currentState = initialState;
    }

    modifier requireState(
        StateMachine storage _machine,
        State _requiredState
    ) {
        require(_machine.currentState == _requiredState);
        _;
    }

    function initStateMachine(
        StateMachine storage _machine
    ) internal {
        require(_machine.initialized == false);

        _machine.currentState = State.Offline;

        _machine.transitionTable[uint8(State.Offline)] = [ State.InsufficientStake, State.Idle ];
        /*
        and so on ...
        */

        _machine.initialized = true;
    }

}

contract StateMachineUser {
    using StateMachine for StateMachine.StateMachine;

    StateMachine.StateMachine internal stateMachine;

    function currentState() public returns (StateMachine.State) {
        return stateMachine.currentState;
    }

    function StateMachineUser () {
        stateMachine.initStateMachine();
    }

    function someFunction(
        // ...
    ) StateMachine.requireState(StateMachine.State.Idle)
      StateMachine.transitionToState(StateMachine.State.Computing) {
        // some code ...
    }
}

This useful solution is not working for now since there is no way to import modifiers from a library.

@limexp

This comment has been minimized.

Contributor

limexp commented Oct 8, 2017

I would like to work on this.

@axic @frangio

Let's take an overall look. From documentation:

Libraries are similar to contracts, but their purpose is that they are deployed only once at a specific address and their code is reused using the DELEGATECALL

There is no way to use modifiers from libraries directly because there is no such objects in EVM. Of course we can translate them to some kind of library functions and then emulate modifiers substitution, but this is not good as different approach and behaviour introduced for "normal modifiers" and "library modifiers". So inlining is the only/best option.

Now let's look at the libraries in Solidity. Why not to inline everything? Why we need DELEGATECALL at all? I understand that some gas saving during deployment can be reached, but is it enough? Maybe it is time to introduce a new concept? Here is the first sight proposal:

  1. Add externalLibrary object that is equal to current library behaviour, but can also be declared by address, for example: externalLibrary MathLib (0x8812...ed0f);. Source code can also be provided as a reference for function calls. Or current library declaration can be extended with address addition.
  2. There must be an utility and/or step before compilation that can get library metadata information and EVM code from block for specified address.
  3. There also must be a way to clone library code and deploy it with a new address for testing in different networks.
  4. If we inline everything then it is an inlineLibrary, or interface, or namespace. Or current library declaration can be extended to force inlining, but different semantics is not good.

Could you provide a clear point of view on a way how to resolve this issue?

I'm ready to code when you make a decision about implementation approach.

@axic

This comment has been minimized.

Member

axic commented Oct 9, 2017

@limexp thanks! Modifiers work like internal functions, they are just inlined.

The first step of this PR is actually enhancing the parser to support fully qualified names in function definitions, i.e. it doesn't accept the dot currently in:
function f() Library.modifier() {}

@limexp

This comment has been minimized.

Contributor

limexp commented Oct 10, 2017

@axic

For modifiers there is no difference if they are defined in a library or in another contract.

As I understand the function f1() definition must be correct. But what about function f2()?

pragma solidity ^0.4.11;

library libraryName {
	modifier libraryModifier() { _; }
}

contract contractName {
	modifier contractModifier() { _; }
}

contract test {
	modifier localModifier() { _; }

//	function f1() libraryName.libraryModifier { }
//	function f2() contractName.contractModifier { }
	function f3() localModifier { }
}

@limexp

This comment has been minimized.

Contributor

limexp commented Oct 11, 2017

I need a small guidance where is a right place to add tests for this issue?

@limexp

This comment has been minimized.

Contributor

limexp commented Oct 12, 2017

@axic

There are several ways this issue could be resolved. I am new to this project, so please help to select the best approach according to spirit of ethereum/solidity.

  1. Do not change class ModifierInvocation definition, but change Identifier semantics, so it could contain period
    ASTPointer<Identifier> m_modifierName;
  2. Add a new ASTPointer<Identifier> m_modifierLibraryName; attribute into class ModifierInvocation.
  3. For m_modifierName attribute create a new ModifierName type like Identifier, but with "period" semantics.
  4. For m_modifierName attribute create a new ModifierName type like UserDefinedTypeName.
  5. Change m_modifierName to UserDefinedTypeName type.
  6. something else...

P.S. I personally prefer the most general approach nr.4, or the minimum code change nr.1.

@VoR0220

This comment has been minimized.

Contributor

VoR0220 commented Nov 14, 2017

I just ran into this myself. Kind of shocked they aren't already added into the functionality of the language. Modifiers in libraries definitely enable reusable safety features and I really think this should be added in.

@androolloyd

This comment has been minimized.

androolloyd commented Nov 24, 2017

@limexp i don't mind option 2.

@chriseth

This comment has been minimized.

Contributor

chriseth commented Nov 24, 2017

Be aware that modifiers are polymorphic. If you reference a modifier through a base class name, it should be exactly that modifier. If you reference it through a library, this should be the same.

Probably it is best to change Identifier to something new like NamePath and then also use NamePath inside UserDefinedTypeName.

This is basically like 3 but a little more generic.

@axic axic added help wanted and removed help wanted labels Dec 2, 2017

@axic axic removed the up-for-grabs label Dec 18, 2017

@axic axic added the feature label Apr 17, 2018

@axic axic added this to To discuss in Backlog (non-breaking) via automation Jul 28, 2018

@roschler

This comment has been minimized.

roschler commented Nov 29, 2018

Comment to show support for this features, especially since it might help contracts that run out of gas during deployment (i.e. - by splitting out code to different contracts).

@chriseth

This comment has been minimized.

Contributor

chriseth commented Nov 29, 2018

@roschler sorry to disappoint you, but I don't see a way how to call modifiers defined in libraries using delegatecall.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment