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

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

Merged
merged 51 commits into from
Jun 6, 2024

Conversation

facs95
Copy link
Contributor

@facs95 facs95 commented May 5, 2024

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

@github-actions github-actions bot added the proto label May 5, 2024
Copy link
Contributor

@0xstepit 0xstepit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
x/evm/statedb/config.go Outdated Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the tests label May 6, 2024
@facs95
Copy link
Contributor Author

facs95 commented May 6, 2024

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

Copy link
Contributor

@0xstepit 0xstepit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, great job @facs95 !

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

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
proto/ethermint/evm/v1/evm.proto Outdated Show resolved Hide resolved
x/evm/keeper/grpc_query_test.go Outdated Show resolved Hide resolved
x/evm/keeper/grpc_query_test.go Outdated Show resolved Hide resolved
x/evm/keeper/integration_test.go Outdated Show resolved Hide resolved
x/evm/keeper/integration_test.go Outdated Show resolved Hide resolved
x/evm/keeper/integration_test.go Outdated Show resolved Hide resolved
x/evm/keeper/integration_test.go Outdated Show resolved Hide resolved
x/evm/keeper/permissions.go Outdated Show resolved Hide resolved
x/evm/keeper/permissions.go 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
proto/ethermint/evm/v1/evm.proto Outdated Show resolved Hide resolved
@github-actions github-actions bot added the CLI label Jun 3, 2024
x/evm/keeper/opcodes_hooks.go Outdated Show resolved Hide resolved
x/evm/keeper/opcodes_hooks.go Outdated Show resolved Hide resolved
x/evm/keeper/opcodes_hooks.go Show resolved Hide resolved
x/evm/keeper/permissions.go Outdated Show resolved Hide resolved
x/evm/keeper/permissions.go Outdated Show resolved Hide resolved
x/evm/keeper/permissions.go Outdated Show resolved Hide resolved
x/evm/keeper/opcodes_hooks.go Show resolved Hide resolved
x/evm/keeper/opcodes_hooks.go Show resolved Hide resolved
x/evm/keeper/opcodes_hooks.go Show resolved Hide resolved
x/evm/module.go Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
x/evm/types/permissions.go Show resolved Hide resolved
x/evm/types/permissions.go Show resolved Hide resolved
x/evm/types/permissions.go Outdated Show resolved Hide resolved
x/evm/types/permissions.go Outdated Show resolved Hide resolved
@fedekunze fedekunze marked this pull request as draft June 5, 2024 09:48
@facs95 facs95 marked this pull request as ready for review June 6, 2024 12:52
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@fedekunze fedekunze enabled auto-merge (squash) June 6, 2024 12:58
@fedekunze fedekunze merged commit 6938440 into main Jun 6, 2024
45 of 48 checks passed
@fedekunze fedekunze deleted the facs95/create-hook branch June 6, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants