feat(evm): Add PermissionsPolicy for permissioned EVM #2538

facs95 · 2024-05-05T14:42:38Z

Description

Closes: #XXXX

With this we take a similar approach to CosmosWasm to make it possible to make the EVM permissioned. This proposes to have now within the EVM params a permissions field with the following types.

// Permissions defines the permission policy of the EVM
// for creating and calling contracts
type Permissions struct {
	// create defines the permission policy for creating contracts
	Create *PermissionType
	// call defines the permission policy for calling contracts
	Call *PermissionType
}

// PermissionType defines the permission type for policies
type PermissionType struct {
	AccessTypes      AccessType
	WhitelistAddress []string 
}

type AccessType int32
const (
	AccessTypeEverybody        AccessType = 0
	AccessTypeNobody           AccessType = 1
	AccessTypeWhitelistAddress AccessType = 2
)

This allows for a granular control over permissions for create and call opcodes while also making it customizable for other customers of the evmosOS (this will improve in future versions).

In our version of the implementation we are passing this into the opcodeHooks for it to be called within the CREATE and CALL opcodes. If AccessTypeWhitelistAddress is chosen, then we will check first that if the signer of the tx is a whitelisted address, if not then we check if the caller (contracts performing internal calls) has permissions.

	switch permissions.Create.AccessType {
	case types.AccessTypeEverybody:
		return func(caller string) bool { return true }
	case types.AccessTypeNobody:
		return func(caller string) bool { return false }
	case types.AccessTypeWhitelistAddress:
		addresses := permissions.Create.WhitelistAddresses
		isSignerAllowed := slices.Contains(addresses, signer)
		return func(caller string) bool {
			return isSignerAllowed || slices.Contains(addresses, caller)
		}
	}
	return func(caller string) bool { return false }

NOTE: This is blocked by evmos/go-ethereum#28

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

tackled an existing issue or discussed with a team member
left instructions on how to review the changes
targeted the correct branch (see PR Targeting)

Reviewers Checklist

All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.

I have...

added a relevant changelog entry to the Unreleased section in CHANGELOG.md
confirmed all author checklist items have been addressed
confirmed that this PR does not change production code
reviewed content
tested instructions (if applicable)
confirmed all CI checks have passed

0xstepit

Nice approach @facs95! I have one question. If the authorization is associated with the sender, how a contract that requires to create new ERC20 tokens, like an AMM, can work?

proto/ethermint/evm/v1/evm.proto

x/evm/statedb/config.go

x/evm/types/params.go

facs95 · 2024-05-06T23:08:19Z

Reimplemented with a new approach based on @0xstepit suggestions! Please take a look and give me feedback. Will implement tests afterwards. Tested locally and works as expected. I will add a brief description tomorrow to explain it better. cc. @fedekunze

Pay special attention to:

proto files
state_transition.go
permissions.go
opcode_hooks.go

0xstepit

Looks good, great job @facs95 !

GAtom22

Great work @facs95!!

Left a few comments

Also, we'll need to update the module version and add the corresponding store migration

app/ante/evm/ante_test.go

proto/ethermint/evm/v1/evm.proto

x/evm/keeper/grpc_query_test.go

GAtom22 · 2024-05-08T12:18:30Z

x/evm/keeper/integration_test.go

+			res, err := s.factory.ExecuteEthTx(signer.Priv, txArgs)
+			Expect(err).To(BeNil())
+			Expect(res.IsOK()).To(Equal(true), "transaction should have succeeded", res.GetLog())
+		})


Nit: check that the balance was updated accordingly

GAtom22 · 2024-05-08T12:20:54Z

x/evm/keeper/integration_test.go

+			}
+			res, err := s.factory.ExecuteContractCall(signerPriv, txArgs, callArgs)
+			Expect(err).To(BeNil())
+			Expect(res.IsOK()).To(Equal(true), "transaction should have succeeded", res.GetLog())


Nit: validate that the state was updated accordingly

GAtom22 · 2024-05-08T12:21:48Z

x/evm/keeper/integration_test.go

+
+			res, err := s.factory.ExecuteEthTx(signer.Priv, txArgs)
+			Expect(err).To(BeNil())
+			Expect(res.IsOK()).To(Equal(true), "transaction should have succeeded", res.GetLog())


GAtom22 · 2024-05-08T12:22:04Z

x/evm/keeper/integration_test.go

+
+			res, err := s.factory.ExecuteEthTx(signer.Priv, txArgs)
+			Expect(err).To(BeNil())
+			Expect(res.IsOK()).To(Equal(true), "transaction should have succeeded", res.GetLog())


ditto: validate that the balances are updated

GAtom22 · 2024-05-08T12:29:01Z

x/evm/keeper/permissions.go

+	case types.AccessTypeEverybody:
+		return func(_ string) bool { return true }
+	case types.AccessTypeNobody:
+		return func(_ string) bool { return false }
+	case types.AccessTypeWhitelistAddress:
+		addresses := permissions.Create.WhitelistAddresses
+		isSignerAllowed := slices.Contains(addresses, signer)
+		return func(caller string) bool {
+			return isSignerAllowed || slices.Contains(addresses, caller)
+		}
+	}


Nit: encapsulate this logic into a func that takes the accessType & whitelistAddresses as input to avoid repeating the code in getCreateCallerFn and generateCallerFn

GAtom22 · 2024-05-08T12:32:04Z

x/evm/keeper/permissions.go

+	return p.canCall(caller)
+}
+
+func generateCallerFn(permissions *types.Permissions, signer string) callerFn {


this function name is not very clear, can you please refactor it or add some comments about it. Maybe something like getCanCallFn is clearer

proto/ethermint/evm/v1/evm.proto

fedekunze · 2024-05-15T08:45:34Z

proto/ethermint/evm/v1/evm.proto

+  // whitelist_addresses defines which addresses have permissions in case of permissioned access_type
+  repeated string whitelist_addresses = 2


let's rename this to allowlist please

fedekunze · 2024-05-15T08:45:53Z

proto/ethermint/evm/v1/evm.proto

+  option (gogoproto.goproto_enum_prefix) = false;
+
+  // ACCESS_TYPE_PERMISSIONLESS does not restrict the operation to anyone
+  ACCESS_TYPE_PERMISSIONLESS = 0


facs95 added 2 commits May 5, 2024 10:41

feat(evm): Add Create OpHook for permissioned contract deployment

d777147

remove go.mod replae

3e37229

github-actions bot added the proto label May 5, 2024

rename function

af7df3d

0xstepit reviewed May 6, 2024

View reviewed changes

proto/ethermint/evm/v1/evm.proto Outdated Show resolved Hide resolved

fedekunze requested changes May 6, 2024

View reviewed changes

x/evm/statedb/config.go Outdated Show resolved Hide resolved

x/evm/types/params.go Outdated Show resolved Hide resolved

facs95 added 2 commits May 6, 2024 18:37

permission policy implementaion

cc95065

remove enable create and call parmas

e4914b8

github-actions bot added the tests label May 6, 2024

facs95 added 2 commits May 6, 2024 19:06

add validate

6d64ab3

revert go.mod replace

f2a36da

0xstepit reviewed May 7, 2024

View reviewed changes

facs95 changed the title ~~feat(evm): Add Create OpHook for permissioned contract deployment~~ feat(evm): Add PermissionsPolicy for permissioned EVM May 7, 2024

facs95 added 6 commits May 7, 2024 08:42

fix proto names

4d8abb5

update geth version

48d3a43

Merge branch 'main' into facs95/create-hook

8fdbf76

fix lint

fbed929

policy unit tests

6f19306

integration tests

fb27a79

GAtom22 reviewed May 8, 2024

View reviewed changes

fedekunze reviewed May 8, 2024

View reviewed changes

proto/ethermint/evm/v1/evm.proto Outdated Show resolved Hide resolved

fedekunze reviewed May 8, 2024

View reviewed changes

proto/ethermint/evm/v1/evm.proto Outdated Show resolved Hide resolved

proto/ethermint/evm/v1/evm.proto Outdated Show resolved Hide resolved

proto/ethermint/evm/v1/evm.proto Outdated Show resolved Hide resolved

ramacarlucho added 4 commits May 8, 2024 16:12

fix tests

6bb171f

fix test

efeaf68

rename permission to accesscontrol

0dcc9bb

fix params tests

abc88e8

fedekunze reviewed May 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(evm): Add PermissionsPolicy for permissioned EVM #2538

feat(evm): Add PermissionsPolicy for permissioned EVM #2538

facs95 commented May 5, 2024 •

edited

0xstepit left a comment

facs95 commented May 6, 2024 •

edited

0xstepit left a comment

GAtom22 left a comment

GAtom22 May 8, 2024

GAtom22 May 8, 2024

GAtom22 May 8, 2024

GAtom22 May 8, 2024

GAtom22 May 8, 2024

GAtom22 May 8, 2024

fedekunze May 15, 2024

fedekunze May 15, 2024

		// whitelist_addresses defines which addresses have permissions in case of permissioned access_type
		repeated string whitelist_addresses = 2

feat(evm): Add PermissionsPolicy for permissioned EVM #2538

Are you sure you want to change the base?

feat(evm): Add PermissionsPolicy for permissioned EVM #2538

Conversation

facs95 commented May 5, 2024 • edited

Description

Author Checklist

Reviewers Checklist

0xstepit left a comment

Choose a reason for hiding this comment

facs95 commented May 6, 2024 • edited

0xstepit left a comment

Choose a reason for hiding this comment

GAtom22 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

facs95 commented May 5, 2024 •

edited

facs95 commented May 6, 2024 •

edited