Skip to content
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

Adding default implementations to interface methods #989

Closed
bjartek opened this issue Jun 8, 2021 · 5 comments · Fixed by #1076
Closed

Adding default implementations to interface methods #989

bjartek opened this issue Jun 8, 2021 · 5 comments · Fixed by #1076

Comments

@bjartek
Copy link
Contributor

bjartek commented Jun 8, 2021

Issue To Be Solved

In order to evolve contracts and standards like the NFT contract Interface having some way to specify the default behavior of a interface method is needed.

Say you want to add a getName(): String method to all INFT types. In order to do that safely the sanes way is IMHO to add support for default implementations.

Suggested Solution

Interface methods can today contain pre and post blocks. One suggestion from bluesign is that they can also contain a default block. And when implementing an interface methods that have a default block can be left unimplemented in the implementing code

Context

See here for context onflow/flow-nft#9

@turbolent
Copy link
Member

Notes

  • Current state: We already have support for declaring pre/post-conditions in interface function bodies and linking them into execution

  • We want to keep support for function declarations in interfaces that have no body, or just pre and post conditions

    struct interface X {
       fun foo() { only pre, post ... }
       fun bar()
    }
  • We want to add support for function declarations in interfaces with a body

    struct interface X {
        fun foo() {}
    }
  • Parsing (in runtime/parser2):

    • Already done: Interface declarations are parsed like composite declarations
    • Add test
  • Checking (in runtime/sema):

    • Allow function bodies in interface
      • see Checker.checkInterfaceSpecialFunctionBlock
      • Change tests to be valid, e.g. TestCheckInvalidInterfaceWithFunctionImplementation
      • Change test names
    • Check the interface function bodies:
      • Checker.checkInterfaceFunctions calls visitFunctionDeclaration:
        • mustExit flag needs rename/change, e.g. OK if there are only pre and post conditions
        • declare should be true (?), as other functions should be able to refer to the function
        • checkResources should be true
    • Composite conforming to interface
      • Note: Implicit conformance when interface requires concrete implementation
      • see Checker.checkCompositeConformance
      • Missing function? check if interface member is function, and if so if it has a block, not a missing member
      • report of MissingFunctionBodyError should stay
      • Record which interface type has default implementation for composite function. Needed to link in default implementation at run-time in interpreter
    • Edge case: Report an error when multiple interfaces provide a default implementation
    • Checker/sema tests are in runtime/tests/checker
  • Execution:

    • Composite functions are already wrapped with pre/post conditions of the interface
    • A wrapper is type FunctionWrapper = func(inner FunctionValue) FunctionValue.
      • Pseudocode:
        return func(body func) {
            preConditions()
            body()
            postConditions()
        }
        
    • See Interpreter.functionConditionsWrapper
    • How is interface code linked into composite?
      => record which interface for which function provides the default implementation in checker! -> semantic
      • CompositeValue.InitializeFunctions links in functions for composite
      • Declared in ...
        EOF

@j1010001
Copy link
Member

merge after secure cadence release

@bjartek
Copy link
Contributor Author

bjartek commented May 23, 2022

Would it be possible to reconsider and have this as part of that release? If we can do that and at the same time implement getViews and resolveViews in NFT contract it would solve the linking problem that is a very hard one.

@turbolent
Copy link
Member

The code for the release was feature frozen a few months ago and then received security audits, so unfortunately we will not be able to get it into this release, but will definitely try to get it into the next release, given that is is now complete.

@j1010001
Copy link
Member

good to go once we verify there are no security implications, might need adding more test cases.

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

Successfully merging a pull request may close this issue.

3 participants