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

refactor: make Role type a uint64 and use constants #1240

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion precompile/allowlist/allowlist.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,11 @@ var (
func GetAllowListStatus(state contract.StateDB, precompileAddr common.Address, address common.Address) Role {
// Generate the state key for [address]
addressKey := common.BytesToHash(address.Bytes())
return Role(state.GetState(precompileAddr, addressKey))
r, err := RoleFromHash(state.GetState(precompileAddr, addressKey))
if err != nil {
panic(err) // DO NOT MERGE
Copy link
Collaborator

Choose a reason for hiding this comment

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

what should the actual behavior be in this case?

}
return r
}

// SetAllowListRole sets the permissions of [address] to [role] for the precompile
Expand Down
4 changes: 2 additions & 2 deletions precompile/allowlist/event.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func UnpackRoleSetEventData(dataBytes []byte) (Role, error) {
}{}
err := AllowListABI.UnpackIntoInterface(&eventData, "RoleSet", dataBytes)
if err != nil {
return Role{}, err
return 0, err
}
return FromBig(eventData.OldRole)
return RoleFromBig(eventData.OldRole)
}
112 changes: 62 additions & 50 deletions precompile/allowlist/role.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,79 +5,80 @@ package allowlist

import (
"errors"
"fmt"
"math/big"

"github.com/ethereum/go-ethereum/common"
"github.com/holiman/uint256"
)

// 1. NoRole - this is equivalent to common.Hash{} and deletes the key from the DB when set
// 2. EnabledRole - allowed to call the precompile
// 3. Admin - allowed to both modify the allowlist and call the precompile
// 4. Manager - allowed to add and remove only enabled addresses and also call the precompile. (only after Durango)
var (
NoRole = Role(common.BigToHash(common.Big0))
EnabledRole = Role(common.BigToHash(common.Big1))
AdminRole = Role(common.BigToHash(common.Big2))
ManagerRole = Role(common.BigToHash(common.Big3))
// Roles should be incremented and not changed.

ErrInvalidRole = errors.New("invalid role")
// Role mirrors the Solidity enum of the same name.
type Role uint64

// Role constants; they MUST be incremented and MUST NOT be reordered.
const (
// NoRole - this is equivalent to common.Hash{} and deletes the key from the DB when set
NoRole Role = iota
// EnabledRole - allowed to call the precompile
EnabledRole
// AdminRole - allowed to both modify the allowlist and call the precompile
AdminRole
// ManagerRole - allowed to add and remove only enabled addresses and also call the precompile. (only after Durango)
ManagerRole

// firstInvalidRole is useful for loops and other upper-bound checks of
// Role values. See [Role.IsValid].
firstInvalidRole
)

// Enum constants for valid Role
type Role common.Hash
// ErrInvalidRole is returned if an invalid role is encountered. If this error
// is returned along with a [Role] value, said value MUST NOT be used; this
// allows functions to return `Role(0), ErrInvalidRole` without worrying about
// misinterpretation as [NoRole].
var ErrInvalidRole = errors.New("invalid role")

// IsNoRole returns true if [r] indicates no specific role.
func (r Role) IsNoRole() bool {
switch r {
case NoRole:
return true
default:
return false
}
}

// IsAdmin returns true if [r] indicates the permission to modify the allow list.
func (r Role) IsAdmin() bool {
switch r {
case AdminRole:
return true
default:
return false
}
// IsValid returns whether or not `r` is a valid value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: while ` is more broadly used avalanche code bases use [] for this type of notation.

func (r Role) IsValid() bool {
return r < firstInvalidRole
}

// IsEnabled returns true if [r] indicates that it has permission to access the resource.
func (r Role) IsEnabled() bool {
switch r {
case AdminRole, EnabledRole, ManagerRole:
return true
default:
return false
}
return r != NoRole && r.IsValid()
}

func (r Role) CanModify(from, target Role) bool {
switch r {
case AdminRole:
return true
case ManagerRole:
return (from == EnabledRole || from == NoRole) && (target == EnabledRole || target == NoRole)
return from.canBeManaged() && target.canBeManaged()
default:
return false
}
}

// canBeManaged returns whether an account with the `Manager` Role can modify
// another account's Role to or from `r`.
func (r Role) canBeManaged() bool {
return r == EnabledRole || r == NoRole
}

func (r Role) uint256() *uint256.Int {
return uint256.NewInt(uint64(r))
}

func (r Role) Bytes() []byte {
return common.Hash(r).Bytes()
b := r.uint256().Bytes32()
return b[:]
}

func (r Role) Big() *big.Int {
return common.Hash(r).Big()
return r.uint256().ToBig()
}

func (r Role) Hash() common.Hash {
return common.Hash(r)
return common.Hash(r.uint256().Bytes32())
}

func (r Role) GetSetterFunctionName() (string, error) {
Expand Down Expand Up @@ -107,16 +108,27 @@ func (r Role) String() string {
case AdminRole:
return "AdminRole"
default:
return "UnknownRole"
return fmt.Sprintf("UnknownRole[%d]", r)
}
}

func FromBig(b *big.Int) (Role, error) {
role := Role(common.BigToHash(b))
switch role {
case NoRole, EnabledRole, ManagerRole, AdminRole:
return role, nil
default:
return Role{}, ErrInvalidRole
// A Big value may or may not be representable as a uint64. Such types include
// [big.Int] and [uint256.Int].
type Big interface {
Uint64() uint64
IsUint64() bool
}

// RoleFromBig converts `u.Uint64()` into a [Role] if `u.IsUint64()` is true.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit

Suggested change
// RoleFromBig converts `u.Uint64()` into a [Role] if `u.IsUint64()` is true.
// RoleFromBig converts `b.Uint64()` into a [Role] if `b.IsUint64()` is true.

func RoleFromBig(b Big) (Role, error) {
r := Role(b.Uint64())
if !b.IsUint64() || !r.IsValid() {
return r, ErrInvalidRole
}
return r, nil
}

// RoleFromHash converts `h` into a [Role].
func RoleFromHash(h common.Hash) (Role, error) {
return RoleFromBig(new(uint256.Int).SetBytes32(h[:]))
}
33 changes: 4 additions & 29 deletions precompile/allowlist/role_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,35 +10,6 @@ import (
"github.com/stretchr/testify/require"
)

func TestIsNoRole(t *testing.T) {
tests := []struct {
role Role
expected bool
}{
{
role: ManagerRole,
expected: false,
},
{
role: AdminRole,
expected: false,
},
{
role: EnabledRole,
expected: false,
},
{
role: NoRole,
expected: true,
},
}

for index, test := range tests {
isNoRole := test.role.IsNoRole()
require.Equal(t, test.expected, isNoRole, fmt.Sprintf("test index: %d", index))
}
}

func TestIsEnabled(t *testing.T) {
tests := []struct {
role Role
Expand All @@ -60,6 +31,10 @@ func TestIsEnabled(t *testing.T) {
role: NoRole,
expected: false,
},
{
role: firstInvalidRole,
expected: false,
},
}

for index, test := range tests {
Expand Down
6 changes: 3 additions & 3 deletions precompile/allowlist/test_allowlist.go
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ func AllowListTests(t testing.TB, module modules.Module) map[string]testutils.Pr
},
SuppliedGas: ReadAllowListGasCost,
ReadOnly: false,
ExpectedRes: common.Hash(NoRole).Bytes(),
ExpectedRes: NoRole.Bytes(),
},
"admin role read allow list": {
Caller: TestAdminAddr,
Expand All @@ -511,7 +511,7 @@ func AllowListTests(t testing.TB, module modules.Module) map[string]testutils.Pr
return input
}, SuppliedGas: ReadAllowListGasCost,
ReadOnly: false,
ExpectedRes: common.Hash(AdminRole).Bytes(),
ExpectedRes: AdminRole.Bytes(),
},
"admin read allow list with readOnly enabled": {
Caller: TestAdminAddr,
Expand All @@ -523,7 +523,7 @@ func AllowListTests(t testing.TB, module modules.Module) map[string]testutils.Pr
return input
}, SuppliedGas: ReadAllowListGasCost,
ReadOnly: true,
ExpectedRes: common.Hash(NoRole).Bytes(),
ExpectedRes: NoRole.Bytes(),
},
"radmin read allow list out of gas": {
Caller: TestAdminAddr,
Expand Down
Loading