-
Notifications
You must be signed in to change notification settings - Fork 270
Sync coreth test cleanup v2 #1623
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
base: master
Are you sure you want to change the base?
Conversation
@@ -38,7 +39,13 @@ var ( | |||
} | |||
|
|||
TestChainConfig = &ChainConfig{ | |||
AvalancheContext: AvalancheContext{SnowCtx: utils.TestSnowContext()}, | |||
AvalancheContext: AvalancheContext{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone with additional context could explain why this is necessary here, but was able to be removed in https://github.com/ava-labs/coreth/pull/950/files#r2080300565, that would be appreciated. Did I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only difference is in Verify() where we pass c.SnowCtx.NetworkUpgrades. I think we should explicitly handle that in tests.
require.NoError(t, err) | ||
require.EqualValues(t, feeConfig, testHighFeeConfig) | ||
require.Zero(t, restartedVM.blockChain.CurrentBlock().Number.Cmp(lastChangedAt)) | ||
require.EqualValues(t, restartedVM.blockChain.CurrentBlock().Number, lastChangedAt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know why this changed or why it worked before. Again would appreciate another set of eyes and brain!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines are functionally identical except that EqualValues assumes type compatibility?? Really lost as to why this worked.
plugin/evm/vm.go
Outdated
@@ -149,6 +149,7 @@ var ( | |||
|
|||
var ( | |||
errEmptyBlock = errors.New("empty block") | |||
errUnsupportedFXs = errors.New("unsupported feature extensions") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subnet-evm does upgrades
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I frankly don't know why tests are broken and why it does not show anything. but it looks like something is broken in plugin/evm/ tests
@@ -93,19 +93,19 @@ func (n *NetworkUpgrades) SetDefaults(agoUpgrades upgrade.Config) { | |||
func (n *NetworkUpgrades) verifyNetworkUpgrades(agoUpgrades upgrade.Config) error { | |||
defaults := GetNetworkUpgrades(agoUpgrades) | |||
if err := verifyWithDefault(n.SubnetEVMTimestamp, defaults.SubnetEVMTimestamp); err != nil { | |||
return fmt.Errorf("SubnetEVM fork block timestamp is invalid: %w", err) | |||
return fmt.Errorf("subnetEVM fork block timestamp is invalid: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we revert these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a linting error that error comments should not be capitalized. Still revert?
https://staticcheck.dev/docs/checks#ST1005
I guess the checker doesn't know they're proper nouns?
@@ -38,7 +39,13 @@ var ( | |||
} | |||
|
|||
TestChainConfig = &ChainConfig{ | |||
AvalancheContext: AvalancheContext{SnowCtx: utils.TestSnowContext()}, | |||
AvalancheContext: AvalancheContext{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only difference is in Verify() where we pass c.SnowCtx.NetworkUpgrades. I think we should explicitly handle that in tests.
testMinGasPrice int64 = 225_000_000_000 | ||
testKeys []*ecdsa.PrivateKey | ||
testMinGasPrice int64 = 225_000_000_000 | ||
testKeys = secp256k1.TestKeys()[:2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be secp256k1.TestKeys()[:3]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, unlike coreth, only, the highest index of any key used is [1], so only 2 keys are necessary.
testEthAddrs []common.Address // testEthAddrs[i] corresponds to testKeys[i] | ||
|
||
firstTxAmount = new(big.Int).Mul(big.NewInt(testMinGasPrice), big.NewInt(21000*100)) | ||
|
||
genesisJSON = func(cfg *params.ChainConfig) string { | ||
toGenesisJSON = func(cfg *params.ChainConfig) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was genesisJSON
in coreth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is; however there are multiple declarations of genesisJSON as a string, which leads to namespace collisions, so I renamed the function.
upgradetest.Granite: params.TestGraniteChainConfig, | ||
} | ||
|
||
genesisJSONPreSubnetEVM string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we initialize them here as genesisJSONPreSubnetEVM = toGenesisJSON(params.TestPreSubnetEVMChainConfig)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this but couldn't figure out a good way to do it - I initialized them in init()
after the new ETH addresses were funded. If I did so before funding, I would get test errors about insufficient balances. In the prior rendition, static addresses were used which allowed them to be funded ahead of time.
|
||
return ctx | ||
} | ||
|
||
func NewTestValidatorState() *validatorstest.State { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should remove this entire file
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestNewTestSnowContext(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test should be in utilstest
func setupGenesis( | ||
t *testing.T, | ||
fork upgradetest.Fork, | ||
fallbackGenesisJSON string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should not need that and just use genesisJson option in newVM
@@ -99,7 +101,7 @@ func TestValidatorState(t *testing.T) { | |||
vm = &VM{} | |||
err = vm.Initialize( | |||
context.Background(), | |||
utils.TestSnowContext(), // this context does not have validators state, making VM to source it from the database | |||
utilstest.NewTestSnowContext(t, snowtest.CChainID), // this context does not have validators state, making VM to source it from the database |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not use CChainID here and generate a new ID. If we're only using a single chain ID (CChainID) then I think we can just skip taking a ChainID in NewTestSnowContext
and just define a subnet-evm chain ID and use that.
Largely mirrors ava-labs/coreth#950, but this time I don't make mistakes 😎
Do note that this PR goes beyond a simple drop in and replace in from ava-labs/coreth#950 as the testing framework upgrade had implications for tests and functionality not included in coreth (which is why I messed up on first revision)
No breaking changes
Does not require release notes