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

Rename configs: alternative #520

Merged
merged 5 commits into from
Feb 17, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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: 3 additions & 3 deletions params/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import (

"github.com/ava-labs/avalanchego/snow"
"github.com/ava-labs/subnet-evm/commontype"
"github.com/ava-labs/subnet-evm/precompile/config"
precompileConfig "github.com/ava-labs/subnet-evm/precompile/config"
"github.com/ava-labs/subnet-evm/precompile/modules"
"github.com/ava-labs/subnet-evm/utils"
"github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -558,7 +558,7 @@ type Rules struct {
// for this rule set.
// Note: none of these addresses should conflict with the address space used by
// any existing precompiles.
ActivePrecompiles map[common.Address]config.Config
ActivePrecompiles map[common.Address]precompileConfig.Config
}

// IsPrecompileEnabled returns true if the precompile at [addr] is enabled for this rule set.
Expand Down Expand Up @@ -594,7 +594,7 @@ func (c *ChainConfig) AvalancheRules(blockNum, blockTimestamp *big.Int) Rules {
rules.IsSubnetEVM = c.IsSubnetEVM(blockTimestamp)

// Initialize the stateful precompiles that should be enabled at [blockTimestamp].
rules.ActivePrecompiles = make(map[common.Address]config.Config)
rules.ActivePrecompiles = make(map[common.Address]precompileConfig.Config)
for _, module := range modules.RegisteredModules() {
if config := c.GetActivePrecompileConfig(module.Address, blockTimestamp); config != nil && !config.IsDisabled() {
rules.ActivePrecompiles[module.Address] = config
Expand Down
9 changes: 4 additions & 5 deletions params/precompile_upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"fmt"
"math/big"

"github.com/ava-labs/subnet-evm/precompile/config"
precompileConfig "github.com/ava-labs/subnet-evm/precompile/config"
"github.com/ava-labs/subnet-evm/precompile/modules"
"github.com/ava-labs/subnet-evm/utils"
Expand All @@ -23,7 +22,7 @@ var errNoKey = errors.New("PrecompileUpgrade cannot be empty")
// based on the key. Keys are defined in each precompile module, and registered in
// precompile/registry/registry.go.
type PrecompileUpgrade struct {
config.Config
precompileConfig.Config
}

// UnmarshalJSON unmarshals the json into the correct precompile config type
Expand Down Expand Up @@ -152,7 +151,7 @@ func (c *ChainConfig) verifyPrecompileUpgrades() error {

// GetActivePrecompileConfig returns the most recent precompile config corresponding to [address].
// If none have occurred, returns nil.
func (c *ChainConfig) GetActivePrecompileConfig(address common.Address, blockTimestamp *big.Int) config.Config {
func (c *ChainConfig) GetActivePrecompileConfig(address common.Address, blockTimestamp *big.Int) precompileConfig.Config {
configs := c.GetActivatingPrecompileConfigs(address, nil, blockTimestamp, c.PrecompileUpgrades)
if len(configs) == 0 {
return nil
Expand All @@ -162,13 +161,13 @@ func (c *ChainConfig) GetActivePrecompileConfig(address common.Address, blockTim

// GetActivatingPrecompileConfigs returns all upgrades configured to activate during the state transition from a block with timestamp [from]
// to a block with timestamp [to].
func (c *ChainConfig) GetActivatingPrecompileConfigs(address common.Address, from *big.Int, to *big.Int, upgrades []PrecompileUpgrade) []config.Config {
func (c *ChainConfig) GetActivatingPrecompileConfigs(address common.Address, from *big.Int, to *big.Int, upgrades []PrecompileUpgrade) []precompileConfig.Config {
// Get key from address.
module, ok := modules.GetPrecompileModuleByAddress(address)
if !ok {
return nil
}
configs := make([]config.Config, 0)
configs := make([]precompileConfig.Config, 0)
key := module.ConfigKey
// First check the embedded [upgrade] for precompiles configured
// in the genesis chain config.
Expand Down
4 changes: 2 additions & 2 deletions params/precompiles.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ package params
import (
"encoding/json"

"github.com/ava-labs/subnet-evm/precompile/config"
precompileConfig "github.com/ava-labs/subnet-evm/precompile/config"
"github.com/ava-labs/subnet-evm/precompile/modules"
)

type Precompiles map[string]config.Config
type Precompiles map[string]precompileConfig.Config

// UnmarshalJSON parses the JSON-encoded data into the ChainConfigPrecompiles.
// ChainConfigPrecompiles is a map of precompile module keys to their
Expand Down
6 changes: 3 additions & 3 deletions precompile/allowlist/allow_list_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func TestAllowListRun(t *testing.T) {
expectedRes []byte
expectedErr string

config *Config
config *AllowList

assertState func(t *testing.T, state *state.StateDB)
}
Expand Down Expand Up @@ -221,7 +221,7 @@ func TestAllowListRun(t *testing.T) {
expectedErr: vmerrs.ErrOutOfGas.Error(),
},
"initial config sets admins": {
config: &Config{
config: &AllowList{
AdminAddresses: []common.Address{noRoleAddr, enabledAddr},
},
suppliedGas: 0,
Expand All @@ -233,7 +233,7 @@ func TestAllowListRun(t *testing.T) {
},
},
"initial config sets enabled": {
config: &Config{
config: &AllowList{
EnabledAddresses: []common.Address{noRoleAddr, adminAddr},
},
suppliedGas: 0,
Expand Down
10 changes: 5 additions & 5 deletions precompile/allowlist/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@ import (
"github.com/ethereum/go-ethereum/common"
)

// Config specifies the initial set of addresses with Admin or Enabled roles.
type Config struct {
// AllowList specifies the initial set of addresses with Admin or Enabled roles.
type AllowList struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer this to be AllowListConfig, since AllowList is more like the collection of these contract functions.

darioush marked this conversation as resolved.
Show resolved Hide resolved
AdminAddresses []common.Address `json:"adminAddresses,omitempty"` // initial admin addresses
EnabledAddresses []common.Address `json:"enabledAddresses,omitempty"` // initial enabled addresses
}

// Configure initializes the address space of [precompileAddr] by initializing the role of each of
// the addresses in [AllowListAdmins].
func (c *Config) Configure(state contract.StateDB, precompileAddr common.Address) error {
func (c *AllowList) Configure(state contract.StateDB, precompileAddr common.Address) error {
for _, enabledAddr := range c.EnabledAddresses {
SetAllowListRole(state, precompileAddr, enabledAddr, EnabledRole)
}
Expand All @@ -29,7 +29,7 @@ func (c *Config) Configure(state contract.StateDB, precompileAddr common.Address
}

// Equal returns true iff [other] has the same admins in the same order in its allow list.
func (c *Config) Equal(other *Config) bool {
func (c *AllowList) Equal(other *AllowList) bool {
if other == nil {
return false
}
Expand All @@ -52,7 +52,7 @@ func areEqualAddressLists(current []common.Address, other []common.Address) bool
}

// Verify returns an error if there is an overlapping address between admin and enabled roles
func (c *Config) Verify() error {
func (c *AllowList) Verify() error {
addressMap := make(map[common.Address]Role) // tracks which addresses we have seen and their role

// check for duplicates in enabled list
Expand Down
32 changes: 16 additions & 16 deletions precompile/allowlist/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,32 +10,32 @@ import (
"github.com/stretchr/testify/require"
)

func TestVerifyAllowlistConfig(t *testing.T) {
func TestVerifyAllowlistAllowList(t *testing.T) {
admins := []common.Address{{1}}
enableds := []common.Address{{2}}
tests := []struct {
name string
config Config
config AllowList
expectedError string
}{
{
name: "invalid allow list config in allowlist",
config: Config{admins, admins},
config: AllowList{admins, admins},
expectedError: "cannot set address",
},
{
name: "nil member allow list config in allowlist",
config: Config{nil, nil},
config: AllowList{nil, nil},
expectedError: "",
},
{
name: "empty member allow list config in allowlist",
config: Config{[]common.Address{}, []common.Address{}},
config: AllowList{[]common.Address{}, []common.Address{}},
expectedError: "",
},
{
name: "valid allow list config in allowlist",
config: Config{admins, enableds},
config: AllowList{admins, enableds},
expectedError: "",
},
}
Expand All @@ -53,37 +53,37 @@ func TestVerifyAllowlistConfig(t *testing.T) {
}
}

func TestEqualAllowListConfig(t *testing.T) {
func TestEqualAllowListAllowList(t *testing.T) {
admins := []common.Address{{1}}
enableds := []common.Address{{2}}
tests := []struct {
name string
config *Config
other *Config
config *AllowList
other *AllowList
expected bool
}{
{
name: "non-nil config and nil other",
config: &Config{admins, enableds},
config: &AllowList{admins, enableds},
other: nil,
expected: false,
},
{
name: "different admin",
config: &Config{admins, enableds},
other: &Config{[]common.Address{{3}}, enableds},
config: &AllowList{admins, enableds},
other: &AllowList{[]common.Address{{3}}, enableds},
expected: false,
},
{
name: "different enabled",
config: &Config{admins, enableds},
other: &Config{admins, []common.Address{{3}}},
config: &AllowList{admins, enableds},
other: &AllowList{admins, []common.Address{{3}}},
expected: false,
},
{
name: "same config",
config: &Config{admins, enableds},
other: &Config{admins, enableds},
config: &AllowList{admins, enableds},
other: &AllowList{admins, enableds},
expected: true,
},
}
Expand Down
2 changes: 1 addition & 1 deletion precompile/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// See the file LICENSE for licensing terms.

// Defines the stateless interface for unmarshalling an arbitrary config of a precompile
package config
package precompileConfig
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if we want to change the package name, we should also change the folder name

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not %100 sure about Go best practices but I feel like package name should be precompileconfig

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree it should be all lowercase. Generally, I think multi-word package names are discouraged. I'd prefer to leave as config and import under the alias precompileConfig, but maybe that's too much manual work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried that before but I think the lowercase option is better (not a fan of aliasing), feel free to have a look at the previous commit and lmk what you think @aaronbuchwald .

This package is going to be a bit hard to fit with the standard rules.


import (
"math/big"
Expand Down
3 changes: 1 addition & 2 deletions precompile/config/mock_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
// See the file LICENSE for licensing terms.

// TODO: replace with gomock

package config
package precompileConfig

import (
"math/big"
Expand Down
2 changes: 1 addition & 1 deletion precompile/config/upgradeable.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// (c) 2022 Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package config
package precompileConfig

import (
"math/big"
Expand Down
6 changes: 3 additions & 3 deletions precompile/contract/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (

"github.com/ava-labs/avalanchego/snow"
"github.com/ava-labs/subnet-evm/commontype"
"github.com/ava-labs/subnet-evm/precompile/config"
precompileConfig "github.com/ava-labs/subnet-evm/precompile/config"
"github.com/ethereum/go-ethereum/common"
)

Expand Down Expand Up @@ -69,10 +69,10 @@ type BlockContext interface {
}

type Configurator interface {
NewConfig() config.Config
NewConfig() precompileConfig.Config
Configure(
chainConfig ChainConfig,
precompileConfig config.Config,
precompileConfig precompileConfig.Config,
state StateDB,
blockContext BlockContext,
) error
Expand Down
4 changes: 2 additions & 2 deletions precompile/contract/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
"testing"

"github.com/ava-labs/subnet-evm/accounts/abi"
"github.com/ava-labs/subnet-evm/precompile/config"
precompileConfig "github.com/ava-labs/subnet-evm/precompile/config"
"github.com/ava-labs/subnet-evm/vmerrs"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/crypto"
Expand Down Expand Up @@ -39,7 +39,7 @@ type PrecompileTest struct {
// It should be the same precompile config that is used in the
// precompile's configurator.
// If nil, Configure on the Configurator will not be called.
Config config.Config
Config precompileConfig.Config
// BeforeHook is called before the precompile is called.
BeforeHook func(t *testing.T, state StateDB)
// AfterHook is called after the precompile is called.
Expand Down
20 changes: 10 additions & 10 deletions precompile/contracts/deployerallowlist/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,36 +7,36 @@ import (
"math/big"

"github.com/ava-labs/subnet-evm/precompile/allowlist"
"github.com/ava-labs/subnet-evm/precompile/config"
precompileConfig "github.com/ava-labs/subnet-evm/precompile/config"
"github.com/ethereum/go-ethereum/common"
)

var _ config.Config = &Config{}
var _ precompileConfig.Config = &Config{}

// Config contains the configuration for the ContractDeployerAllowList precompile,
// consisting of the initial allowlist and the timestamp for the network upgrade.
type Config struct {
allowlist.Config
config.Upgrade
allowlist.AllowList
precompileConfig.Upgrade
}

// NewConfig returns a config for a network upgrade at [blockTimestamp] that enables
// ContractDeployerAllowList with [admins] and [enableds] as members of the allowlist.
func NewConfig(blockTimestamp *big.Int, admins []common.Address, enableds []common.Address) *Config {
return &Config{
Config: allowlist.Config{
AllowList: allowlist.AllowList{
AdminAddresses: admins,
EnabledAddresses: enableds,
},
Upgrade: config.Upgrade{BlockTimestamp: blockTimestamp},
Upgrade: precompileConfig.Upgrade{BlockTimestamp: blockTimestamp},
}
}

// NewDisableConfig returns config for a network upgrade at [blockTimestamp]
// that disables ContractDeployerAllowList.
func NewDisableConfig(blockTimestamp *big.Int) *Config {
return &Config{
Upgrade: config.Upgrade{
Upgrade: precompileConfig.Upgrade{
BlockTimestamp: blockTimestamp,
Disable: true,
},
Expand All @@ -46,13 +46,13 @@ func NewDisableConfig(blockTimestamp *big.Int) *Config {
func (*Config) Key() string { return ConfigKey }

// Equal returns true if [cfg] is a [*ContractDeployerAllowListConfig] and it has been configured identical to [c].
func (c *Config) Equal(cfg config.Config) bool {
func (c *Config) Equal(cfg precompileConfig.Config) bool {
// typecast before comparison
other, ok := (cfg).(*Config)
if !ok {
return false
}
return c.Upgrade.Equal(&other.Upgrade) && c.Config.Equal(&other.Config)
return c.Upgrade.Equal(&other.Upgrade) && c.AllowList.Equal(&other.AllowList)
}

func (c *Config) Verify() error { return c.Config.Verify() }
func (c *Config) Verify() error { return c.AllowList.Verify() }
Loading