refactor: params/extras (coreth a7f7061) #1430
Conversation
| @@ -15,8 +16,7 @@ import ( | |||
|
|
|||
| func TestVerifyUpgradeConfig(t *testing.T) { | |||
| admins := []common.Address{{1}} | |||
| chainConfigCpy := Copy(TestChainConfig) | |||
| chainConfig := GetExtra(&chainConfigCpy) | |||
| chainConfig := &ChainConfig{} | |||
There was a problem hiding this comment.
note I tried to use the empty ChainConfig (former ChainConfigExtra) where possible
We can switch back to TestChainConfig if others prefer.
Less test setup seems preferred to me, but we could keep it to avoid any risk here.
| AvalancheContext: AvalancheContext{utils.TestSnowContext()}, | ||
| FeeConfig: DefaultFeeConfig, | ||
| AllowFeeRecipients: false, | ||
| NetworkUpgrades: GetDefaultNetworkUpgrades(upgrade.GetConfig(constants.UnitTestID)), // This can be changed to correct network (local, test) via VM. |
There was a problem hiding this comment.
note we could unexport this GetDefaultNetworkUpgrades if we had something like GetDefaultExtras(networkID), but seems mostly the same to me.
| // IsForkTransition returns true if [fork] activates during the transition from | ||
| // [parent] to [current]. | ||
| // Taking [parent] as a pointer allows for us to pass nil when checking forks | ||
| // that activate during genesis. | ||
| // Note: [parent] and [current] can be either both timestamp values, or both |
There was a problem hiding this comment.
nit - maybe out of scope - but brackets only work for type definitions or global variables, they won't work for arguments 🤷 Maybe worth changing to use backticks instead
| // IsForkTransition returns true if [fork] activates during the transition from | |
| // [parent] to [current]. | |
| // Taking [parent] as a pointer allows for us to pass nil when checking forks | |
| // that activate during genesis. | |
| // Note: [parent] and [current] can be either both timestamp values, or both | |
| // IsForkTransition returns true if `fork` activates during the transition from | |
| // `parent` to `current`. | |
| // Taking `parent` as a pointer allows for us to pass nil when checking forks | |
| // that activate during genesis. | |
| // Note: `parent` and `current` can be either both timestamp values, or both |
There was a problem hiding this comment.
Using the brackets seems very specific to the avalanche code bases, I would appreciate if you were able to figure out the best practices here and communicate them with the team, especially for code on a go-forward basis.
There was a problem hiding this comment.
See https://go.dev/doc/comment#doclinks - if it's not cmd-clickable, don't use square brackets since it's rather confusing to have clickable elements and some not despite the same brackets syntax.
| return nil | ||
| } | ||
|
|
||
| // IsPrecompileEnabled returns whether precompile with [address] is enabled at [timestamp]. |
There was a problem hiding this comment.
nit
| // IsPrecompileEnabled returns whether precompile with [address] is enabled at [timestamp]. | |
| // IsPrecompileEnabled returns whether precompile with `address` is enabled at `timestamp`. |
|
|
||
| // checkForks checks that forks are enabled in order and returns an error if not. | ||
| // [blockFork] is true if the fork is a block number fork, false if it is a timestamp fork | ||
| // TODO: This code was adapted from CheckConfigForkOrder, consider refactoring to avoid duplication. |
There was a problem hiding this comment.
New TODO added, just in case it was forgotten - otherwise discard my comment 😉
| } | ||
|
|
||
| // checkForks checks that forks are enabled in order and returns an error if not. | ||
| // [blockFork] is true if the fork is a block number fork, false if it is a timestamp fork |
There was a problem hiding this comment.
nit brackets don't work for function arguments
| // [blockFork] is true if the fork is a block number fork, false if it is a timestamp fork | |
| // `blockFork` is true if the fork is a block number fork, false if it is a timestamp fork |
Applies the same changes as were applied to coreth here: ava-labs/coreth@a7f7061
I realized we could import most of these types back to params if we wanted, but explicit seems clearer + it's already like this in coreth.