Introduce more strongly typed SNatives and contract templates #500

silasdavis · 2017-02-17T01:27:48Z

This PR aims to address the issue of having many versions of the permissions contract in different places. Since eris-db is responsible for interpreting SNative calls it seems sensible that eris-db holds the defintion for SNatives. This PR:

Introduces a more strongly typed (the functions themselves are still not strongly typed) SNative and contract definition
Allows you to generate a permissions contract with comments that compiles and can be exported to other projects like smart contract bundles
Adds a test for the permissions contract

It doesn't change the fundamentals of how SNatives work.

The permissions contract structure is built from the libraries' team's version: https://github.com/eris-ltd/eris-contract-bundles/blob/master/src/commons-auth/contracts/SecureNativeAuthorizations.sol which Jason linked me too which is nicely commented.

Outstanding for this is:

Finalise what types we are going to use for the permissions function
Possibly take opportunity to change the naming conventions as in Update keys in sNatives PermsMap to permit/improve sNatives access from Solidity #417
Add a couple more unit tests

Once that is done then we can mechanical generate the solidity interface that we are advertising to clients and we can do this for any future SNatives (which is why I thought it was worth doing). By running:

vm.SNativeContracts()["permissions_contract"].Solidity()

Also note that the current version of snative.go on release-0.16 is missing a bracket in the signature.

fixes #498
fixes #418
fixes #417

…ract, and test for permissions contract

VoR0220 · 2017-02-17T01:43:35Z

Am in favor of changing the names. So yea...lets make this happen.

silasdavis · 2017-02-17T11:48:32Z

@VoR0220 Can we get some consensus on what the type signatures should actually be also?

VoR0220 · 2017-02-17T12:04:38Z

@silasdavis i think i know what you mean but youre going to need to clarify. I think you mean solidity types. In which case there should be changes. For one, all ints should be uint64s to better align with golang. We should also contemplate switching from bytes32 to string as it could enable longer strings and dynamic ones at that.

silasdavis · 2017-02-17T14:18:22Z

What I need is a decision on:

int or uint64 (sounds like uint64)
bytes32 or string (sounds like bytes32)
function name casing (sounds like camel case)
The name of contract (and therefore its address)

These will be breaking changes, but seeing as it was already not working properly I think we are at liberty to make them if they improve the situation.

Here is the contract as currently defined in snatives.go:

/**
* Interface for managing Secure Native authorizations.
* @dev This Solidity interface describes the functions exposed by the SNative permissions layer in the Monax blockchain (ErisDB).
*/
contract permissions_contract {

/**
* @notice Removes a role from an account
* @param _account account
* @param _role role
* @return result whether role was removed
*/
function rm_role(address _account, bytes32 _role) constant returns (bool result);

/**
* @notice Sets a base authorization for an account
* @param _account account
* @param _authorization base authorization
* @param _value value of base authorization
* @return result value passed in
*/
function set_base(address _account, uint64 _authorization, uint64 _value) constant returns (bool result);

/**
* @notice Indicates whether an account has a base authorization
* @param _account account
* @param _authorization base authorization
* @return result whether account has base authorization set
*/
function has_base(address _account, uint64 _authorization) constant returns (bool result);

/**
* @notice Sets a base authorization for an account to the global (default) value of the base authorization
* @param _account account
* @param _authorization base authorization
* @return authorization base authorization passed in
*/
function unset_base(address _account, uint64 _authorization) constant returns (uint64 authorization);

/**
* @notice Sets global (default) value for a base authorization
* @param _account account
* @param _authorization base authorization
* @param _value value of base authorization
* @return authorization base authorization passed in
*/
function set_global(uint64 _authorization, uint64 _value) constant returns (uint64 authorization);

/**
* @notice Adds a role to an account
* @param _account account
* @param _role role
* @return result whether role was added
*/
function add_role(address _account, bytes32 _role) constant returns (bool result);

/**
* @notice Indicates whether an account has a role
* @param _account account
* @param _role role
* @return result whether account has role
*/
function has_role(address _account, bytes32 _role) constant returns (bool result);

}

VoR0220 · 2017-02-17T14:31:16Z

let me play with turning it to string and get back to you later today. It may very well be worth it to send in a string instead of a bytes32 simply because a string can be longer than 32 bytes (why you would ever make a string like that, IDK, but the liberty is nice).

VoR0220 · 2017-02-17T18:44:53Z

manager/eris-mint/evm/snative_test.go

+	contract := SNativeContracts()["permissions_contract"]
+
+	assertContractFunction(t, contract, "3fbf7da5",
+		"add_role(address,bytes32)")


Thought we said camelCase?

Ah right yeah, I can do that, but it will involve a few more changes. Obviously just a search and replace job in Eris db so I can do it, but it got me to question the wisdom, will do it if you and Jason and Ben think it's good idea

benjaminbollen

Overall very nice work. I do want to raise the concern that the SNative should be (possible) without being Solidity specific. We can come back to this at a later point.

The idea with be to define the Args as LanguageUint64 etc rather than SolidityUint64; the templates should be solidity specific only imo

benjaminbollen · 2017-02-20T11:50:52Z

manager/eris-mint/evm/opcodes/opcodes.go

@@ -378,6 +379,8 @@ func Bytecode(bytelikes ...interface{}) []byte {
 			if int64(bytes[i]) != b {
 				panic(fmt.Sprintf("The int64 %v does not fit inside a byte", b))
 			}
+		case word256.Word256:


benjaminbollen · 2017-02-20T11:58:40Z

manager/eris-mint/evm/snative.go

 }

-//-----------------------------------------------------------------------------
-// snative are native contracts that can access and modify an account's permissions
-
 type SNativeFuncDescription struct {


Strictly speaking, should this not be an interface for which we have a Solidity implementation? The argument is that Solidity is not the only language that can access our Ethereum compatible secure native functions

Second thought / correction: SNativeFuncDescription should not become an interface as suggested above. Rather the SNative definition should be defined with LanguageTypes and the templating should be a package on itself, that produces the solidity code based on the mapping of LanguageType to SolidityType. We can raise this as an issue and proceed as is for now; the net gain in readability is sufficient for me.

Make that ProtoType rather than LanguageType

benjaminbollen · 2017-02-20T12:09:21Z

manager/eris-mint/evm/snative.go

+		fid := f.ID()
+		otherF, ok := fs[fid]
+		if ok {
+			panic(fmt.Errorf("Function with ID %x already defined: %s", fid,


This should not panic, even if it would only be called on initialization;

benjaminbollen · 2017-02-20T12:10:34Z

manager/eris-mint/evm/snative.go

-	if !HasPermission(appState, caller, d.PermFlag) {
-		return nil, ErrInvalidPermission{caller.Address, d.Name}
+	if !HasPermission(appState, caller, function.PermFlag) {
+		return Zero256.Bytes(), ErrInvalidPermission{caller.Address, function.Name}


Good on returning Zero256

benjaminbollen · 2017-02-20T12:38:47Z

manager/eris-mint/evm/snative.go

+// Returns a map of all SNative contracts defined indexed by name
+func SNativeContracts() map[string]SNativeContractDescription {
+	permFlagType := SolidityUint64
+	roleType := SolidityBytes32


Internally we treat roles as strings, but I agree that it is better and sufficient to type it strictly as bytes32; alternatively we should change our impl to treat it as Word256

benjaminbollen · 2017-02-20T14:09:06Z

LGTM. I have raised comments on the unpleasantness of infection of solidity into the evm/snatives; Talked with Silas and he will examine further; although he does not like to call it ProtoTypes :)

…bi package, improve documentation of permissions snatives and improve argument and return types

silasdavis · 2017-02-20T17:56:36Z

The latest commit:

Refactors Solidity-specific code into a util package and introduces a templating wrapper, and some formatting improvements (like proper indenting)
Introduces an abi package under evm to decouple Solidity types and calling conventions from those defined by Ethereum by way of the ABI (this can be one path to future expansion to support an ABI-aware chain more generally)
Improves documentation and naming of variables on contract
Changes the 'value' argument on set_* functions to be a bool as it should be
Changes the return value of *set_* functions to be the resultant permission after the permission operation has applied (rather than echoing back the argument the caller passed in). This retains the primary purpose of the return value as a truthy call success flag.
Reverts the returning of Zero256.Bytes() from Dispatch since error return values should be and are handled by the called (vm.go) here: https://github.com/eris-ltd/eris-db/blob/master/manager/eris-mint/evm/vm.go#L857. If there is an error return then zero is pushed to the stack by the vm so it can't have been the cause of some invalid jump dests observed. Since the existing snatives return nil I think it is confusing to break with go convention and return zero to the stack without clear semantics on what should be done in this error case.

VoR0220 · 2017-02-20T18:43:48Z

Makefile

+# Dumps Solidity interface contracts for SNatives
+.PHONY: snatives
+snatives:
+	@go run ./util/snatives/cmd/main.go


Tried this out, works like a charm. Good work.

VoR0220 · 2017-02-20T18:44:01Z

event/event_cache_test.go

@@ -13,7 +13,7 @@ import (
 	"github.com/stretchr/testify/assert"
 )

-var mockInterval = 10 * time.Millisecond
+var mockInterval = 40 * time.Millisecond


Reason for this?

I had a CI failure, timing here was a little tight anyway, also ugly, but pre-existing and we'll get around to that.

VoR0220 · 2017-02-20T18:44:56Z

manager/eris-mint/evm/abi/types.go

@@ -0,0 +1,27 @@
+package abi


Should clarify in the notes below that this is an ABI for our SNative contracts. That it creates a binding from our native Go Code to Solidity and vice versa, and could further be extended to Java and whatever language the enterprise institutions want.

VoR0220 · 2017-02-20T18:45:57Z

manager/eris-mint/evm/fake_app_state.go

@@ -3,7 +3,6 @@ package vm
 import (
 	"fmt"

-	. "github.com/eris-ltd/eris-db/manager/eris-mint/evm"


VoR0220 · 2017-02-20T18:46:03Z

manager/eris-mint/evm/log_event_test.go

@@ -5,7 +5,6 @@ import (
 	"reflect"
 	"testing"

-	. "github.com/eris-ltd/eris-db/manager/eris-mint/evm"


VoR0220 · 2017-02-20T18:47:10Z

manager/eris-mint/evm/snative.go

-		registeredNativeContracts[LeftPadWord256([]byte("add_role"))] = add_role
-		registeredNativeContracts[LeftPadWord256([]byte("rm_role"))] = rm_role
-	*/
+type SNativeContractDescription struct {


Needs Godoc documentation of what each of the parameters that are exported do.

VoR0220 · 2017-02-20T18:47:16Z

manager/eris-mint/evm/snative.go

-// snative are native contracts that can access and modify an account's permissions
-
-type SNativeFuncDescription struct {
+type SNativeFunctionDescription struct {


VoR0220 · 2017-02-20T18:49:26Z

manager/eris-mint/evm/snative.go

@@ -115,7 +297,7 @@ func has_base(appState AppState, caller *Account, args []byte, gas *int64) (outp
 }

 func set_base(appState AppState, caller *Account, args []byte, gas *int64) (output []byte, err error) {
-	addr, permNum, perm := returnThreeArgs(args)
+	addr, permNum, permVal := returnThreeArgs(args)


much better name ++

VoR0220 · 2017-02-20T18:49:40Z

manager/eris-mint/evm/snative.go

@@ -228,6 +410,10 @@ func ValidPermN(n ptypes.PermFlag) bool {
 	return true
 }

+func resultantPermBytes(basePerms ptypes.BasePermissions) []byte {


VoR0220 · 2017-02-20T18:52:50Z

manager/eris-mint/evm/snative_test.go

+https://ethereum.github.io/browser-solidity yields:
+
+Functions
+3fbf7da5 add_role(address,bytes32)


Need to decide if we're going CamelCase Here...or mixedCase (as is Solidity convention).

VoR0220 · 2017-02-20T18:56:01Z

manager/eris-mint/evm/snative.go

+		* Interface for managing Secure Native authorizations.
+		* @dev This interface describes the functions exposed by the SNative permissions layer in the Monax blockchain (ErisDB).
+		`,
+			"permissions_contract",


permissions_contract or are we doing PermissionsContract? Either way I'm fine. Just clarifying.

VoR0220

Needs a couple of documentation changes and then its GTM.

…le guide, add missing godoc comments, automate signature test

VoR0220 · 2017-02-21T00:42:22Z

Very approved. Merge away.

silasdavis · 2017-02-21T00:43:51Z

In for a penny, in for pound. Have changed case convention of permissions functions to camelCase. Also renamed permission contract to Permissions so the canonical address for it will have changed. Also automated testing a bit more.

The generated solidity for the permissions contract now looks like this:

/**
* Interface for managing Secure Native authorizations.
* @dev This interface describes the functions exposed by the SNative permissions layer in the Monax blockchain (ErisDB).
* @dev These functions can be accessed as if this contract were deployed at the address 0x0000000000000000005065726d697373696f6e73
*/
contract Permissions {
    /**
    * @notice Adds a role to an account
    * @param _account account address
    * @param _role role name
    * @return result whether role was added
    */
    function addRole(address _account, bytes32 _role) constant returns (bool result);

    /**
    * @notice Removes a role from an account
    * @param _account account address
    * @param _role role name
    * @return result whether role was removed
    */
    function removeRole(address _account, bytes32 _role) constant returns (bool result);

    /**
    * @notice Indicates whether an account has a role
    * @param _account account address
    * @param _role role name
    * @return result whether account has role
    */
    function hasRole(address _account, bytes32 _role) constant returns (bool result);

    /**
    * @notice Sets the permission flags for an account. Makes them explicitly set (on or off).
    * @param _account account address
    * @param _permission the base permissions flags to set for the account
    * @param _set whether to set or unset the permissions flags at the account level
    * @return result the resultant permissions flags on the account after the call
    */
    function setBase(address _account, uint64 _permission, bool _set) constant returns (uint64 result);

    /**
    * @notice Unsets the permissions flags for an account. Causes permissions being unset to fall through to global permissions.
    * @param _account account address
    * @param _permission the permissions flags to unset for the account
    * @return result the resultant permissions flags on the account after the call
    */
    function unsetBase(address _account, uint64 _permission) constant returns (uint64 result);

    /**
    * @notice Indicates whether an account has a subset of permissions set
    * @param _account account address
    * @param _permission the permissions flags (mask) to check whether enabled against base permissions for the account
    * @return result whether account has the passed permissions flags set
    */
    function hasBase(address _account, uint64 _permission) constant returns (uint64 result);

    /**
    * @notice Sets the global (default) permissions flags for the entire chain
    * @param _permission the permissions flags to set
    * @param _set whether to set (or unset) the permissions flags
    * @return result the resultant permissions flags on the account after the call
    */
    function setGlobal(uint64 _permission, bool _set) constant returns (uint64 result);
}

benjaminbollen

I would want a response on the Resultant Permissions and why we need it; other than than; let s ping products team on the change to the function names; needs to be updated in tooling too then

benjaminbollen · 2017-02-21T18:12:47Z

manager/eris-mint/state/tx_cache.go

@@ -19,6 +19,8 @@ type TxCache struct {
 	storages map[Tuple256]Word256
 }

+var _ vm.AppState = &TxCache{}


A comment is not unwelcome; but great! thx for adding this line

"TxCache implements vm.AppState", what sort of comment were you after?

optionally yes, it s not entirely obvious otherwise

benjaminbollen · 2017-02-21T18:15:37Z

permission/types/permissions.go

@@ -112,6 +112,12 @@ func (p *BasePermissions) IsSet(ty PermFlag) bool {
 	return p.SetBit&ty > 0
 }

+// Returns the Perms PermFlag masked with SetBit bit field to give the resultant
+// permissions enabled by this BasePermissions
+func (p *BasePermissions) ResultantPerms() PermFlag {


this is not ok as a public function; It can encourage usage of it and it should be clear that effective permissions are those of the global permissions + delta of base permissions for that account. Why would we have this function?

If you want to see what permissions are set at the account level. Knowing effective permissions is also useful I agree.

Given that BasePermissions.Perms and BasePermissions.SetBit are both public fields I'd say this was arguably less obscure in meaning than those. Callers are allowed to interrogate account level permissions and global permissions separately.

benjaminbollen · 2017-02-21T18:16:37Z

util/snatives/templates/solidity_templates.go

@@ -0,0 +1,133 @@
+package templates


Nice ! I like that you moved them to util; even better than having them in evm

VoR0220 · 2017-02-21T19:37:31Z

client/rpc/client_test.go

@@ -147,7 +147,7 @@ func testPermissions(t *testing.T,
 	nonceString := ""

 	_, err := Permissions(nodeClient, keyClient, publicKeyString, addressString,
-		nonceString, "set_base", []string{permAddressString, "root", "true"})
+		nonceString, "setBase", []string{permAddressString, "root", "true"})


If we're going to harden these, now would be the time to add a test for every permission. This way we can effectively gauge what parts are on the jobs failing and what parts are on e-db failing, atleast more easily...currently I'm having trouble deciphering this.

There are extensive unit tests in /manager/eris-mint/state/permissions_test.go that have been passing without failure

That's fine, but working directly on the EVM/SNative is not the same as going through the RPC (as we've seen with #477). I would recommend that there is atleast one of every RPC permission call type included in here and to ensure that they are passing.

benjaminbollen

BasePermissions.Set actually sets many bits at the same time; the comment is misleading and I thought wrong about it; this is good

benjaminbollen · 2017-02-22T12:51:17Z

LGTM

…emplates Introduce more strongly typed SNatives and contract templates

silasdavis added area/evm status/dont merge labels Feb 17, 2017

silasdavis force-pushed the snative-templates branch 2 times, most recently from c320376 to 0fc330b Compare February 17, 2017 01:30

Introduce more strongly typed SNatives, templates for generating cont…

3d1e7ec

…ract, and test for permissions contract

silasdavis force-pushed the snative-templates branch from 0fc330b to 3d1e7ec Compare February 17, 2017 01:31

silasdavis changed the title ~~[WIP] Introduce more strongly typed SNatives and templace~~ [WIP] Introduce more strongly typed SNatives and contract template Feb 17, 2017

silasdavis changed the title ~~[WIP] Introduce more strongly typed SNatives and contract template~~ [WIP] Introduce more strongly typed SNatives and contract templates Feb 17, 2017

silasdavis mentioned this pull request Feb 17, 2017

[SNative] Don't return nil from SNative Dispatch #418

Closed

Silas Davis added 2 commits February 17, 2017 16:14

types update

a26ec7c

Fix snative signature test

aa61e95

silasdavis removed the status/dont merge label Feb 17, 2017

silasdavis changed the title ~~[WIP] Introduce more strongly typed SNatives and contract templates~~ Introduce more strongly typed SNatives and contract templates Feb 17, 2017

VoR0220 reviewed Feb 17, 2017

View reviewed changes

silasdavis force-pushed the snative-templates branch 3 times, most recently from 27b6875 to 1add855 Compare February 17, 2017 19:33

Add test for Dispatch

96df876

silasdavis force-pushed the snative-templates branch from 1add855 to 96df876 Compare February 17, 2017 23:24

silasdavis requested a review from benjaminbollen February 18, 2017 21:59

benjaminbollen reviewed Feb 20, 2017

View reviewed changes

Refactor templating to util, move 'Solidity' types to a more proper a…

9c52e95

…bi package, improve documentation of permissions snatives and improve argument and return types

silasdavis force-pushed the snative-templates branch from dbc97cb to 9c52e95 Compare February 20, 2017 17:51

VoR0220 reviewed Feb 20, 2017

View reviewed changes

VoR0220 approved these changes Feb 20, 2017

View reviewed changes

Change case convention for permissions contract to match solidity sty…

af51be5

…le guide, add missing godoc comments, automate signature test

Include magic SNative contract address in generated solidity code

1f421ff

silasdavis force-pushed the snative-templates branch from 5df55ed to 1f421ff Compare February 21, 2017 01:01

benjaminbollen suggested changes Feb 21, 2017

View reviewed changes

Return effective permissions after the various permissions calls

a48afca

VoR0220 reviewed Feb 21, 2017

View reviewed changes

benjaminbollen approved these changes Feb 22, 2017

View reviewed changes

benjaminbollen merged commit de0c481 into hyperledger-archives:release-0.16 Feb 22, 2017

silasdavis deleted the snative-templates branch February 22, 2017 12:59

benjaminbollen mentioned this pull request Feb 28, 2017

Release eris-db v0.16.0 #519

Merged

This was referenced May 6, 2017

Update keys in sNatives PermsMap to permit/improve sNatives access from Solidity #417

Closed

Change hasBase to return bool #606

Merged

silasdavis pushed a commit to silasdavis/burrow that referenced this pull request Mar 9, 2019

Merge pull request hyperledger-archives#500 from silasdavis/snative-t…

c4fb70e

…emplates Introduce more strongly typed SNatives and contract templates

Introduce more strongly typed SNatives and contract templates #500

Introduce more strongly typed SNatives and contract templates #500

Conversation

silasdavis commented Feb 17, 2017 • edited Loading

VoR0220 commented Feb 17, 2017

silasdavis commented Feb 17, 2017

VoR0220 commented Feb 17, 2017

silasdavis commented Feb 17, 2017 • edited Loading

VoR0220 commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminbollen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminbollen commented Feb 20, 2017

silasdavis commented Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VoR0220 Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VoR0220 left a comment

Choose a reason for hiding this comment

VoR0220 commented Feb 21, 2017

silasdavis commented Feb 21, 2017

benjaminbollen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminbollen left a comment

Choose a reason for hiding this comment

benjaminbollen commented Feb 22, 2017

silasdavis commented Feb 17, 2017 •

edited

Loading

silasdavis commented Feb 17, 2017 •

edited

Loading

silasdavis commented Feb 20, 2017 •

edited

Loading

VoR0220 Feb 20, 2017 •

edited

Loading