Remove duplicate fork definitions #3304
Conversation
| @@ -42,14 +40,7 @@ import ( | |||
| keystoreutils "github.com/ava-labs/avalanchego/vms/components/keystore" | |||
| ) | |||
|
|
|||
| type fork uint8 | |||
| @@ -97,8 +88,6 @@ func init() { | |||
| } | |||
| } | |||
|
|
|||
| type fork uint8 | |||
| @@ -100,8 +93,6 @@ func init() { | |||
|
|
|||
| type stakerStatus uint | |||
|
|
|||
| type fork uint8 | |||
| @@ -90,8 +83,6 @@ func init() { | |||
| } | |||
| } | |||
|
|
|||
| type fork uint8 | |||
| @@ -162,8 +150,6 @@ func init() { | |||
| } | |||
| } | |||
|
|
|||
| type fork uint8 | |||
StephenButtolph
added
testing
upgrade/upgradetest/config.go
Outdated
|
|
||
| // SetConfigTimesTo sets the upgrade time of the provided fork, and all prior | ||
| // forks, to the provided upgradeTime. | ||
| func SetConfigTimesTo(c *upgrade.Config, fork Fork, upgradeTime time.Time) { |
There was a problem hiding this comment.
The signature looks like this to align with a pointer receiver method on the Config type (which can't be done here because this is a testing package)
| // SetTimesTo sets the upgrade time of the provided fork, and all prior forks, | ||
| // to the provided upgradeTime. | ||
| func SetTimesTo(c *upgrade.Config, fork Fork, upgradeTime time.Time) { | ||
| switch fork { |
| UpgradeConfig: upgrade.Config{ | ||
| BanffTime: mockable.MaxTime, // banff is not activated | ||
| }, | ||
| UpgradeConfig: upgradetest.GetConfig(upgradetest.ApricotPhasePost6), |
There was a problem hiding this comment.
I really like this pattern. It's much more declarative around intent / effect.
| ApricotPhase4MinPChainHeight: 0, | ||
| DurangoTime: mockable.MaxTime, | ||
| }, | ||
| Upgrades: upgradetest.GetConfigWithUpgradeTime(upgradetest.ApricotPhase4, time.Time{}), |
There was a problem hiding this comment.
Does the (Go) zero-value time.Time{} have a special meaning?
And the Unix zero-value? Is this effectively the same as using upgrade.InitiallyActiveTime?
There was a problem hiding this comment.
These values seem to be depended on in the tests that use this code... I'm hoping that we can remove the reliance on time.Time{} and time.Unix(0, 0) as we continue to fix these tests... But it felt out of scope for this PR...
TL;DR: I don't know... Changing them breaks the tests... The tests should be improved in a later PR.
upgrade/upgradetest/config.go
Outdated
| // be unscheduled. | ||
| func GetConfigWithUpgradeTime(fork Fork, upgradeTime time.Time) upgrade.Config { | ||
| c := upgrade.Config{ | ||
| ApricotPhase4MinPChainHeight: 0, |
There was a problem hiding this comment.
this is the zero value so why explicitly set it?
There was a problem hiding this comment.
We could remove this - I felt like it was a useful shout-out... But I'm fine deleting it.
vms/proposervm/vm_test.go
Outdated
| ApricotPhase4MinPChainHeight: 0, | ||
| DurangoTime: time.Unix(0, 0), | ||
| }, | ||
| Upgrades: upgradetest.GetConfigWithUpgradeTime(upgradetest.Latest, time.Unix(0, 0)), |
There was a problem hiding this comment.
I know it's no the same, as one is 1970 and the other is just 0, but, why the discrepancy between time.Unix(0,0) and time.Time? Won't this test work with time.Time{} ?
There was a problem hiding this comment.
i mean - maybe it's better to be consistent?
There was a problem hiding this comment.
I did this to keep the same values, but you're right time.Time{} seems to work as well. I switched it to use that for consistency.
Why this should be merged
type fork uint8seems to be very popular... Moving this into a common package and sharing where possible.How this works
Adds an
upgradetestpackage and exposes common functionality that existing VMs re-implement multiple times.How this was tested