-
Notifications
You must be signed in to change notification settings - Fork 179
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
Expose minimum required version to cadence interface #6560
Changes from 13 commits
a79c9b8
f82f503
f36aafe
59269e0
fe18d5a
4166e0a
b2fa1c3
2109f64
a72cd52
2ccf370
d9d5ef4
9c47667
456e8a6
58e84cd
b2ee0e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -62,6 +62,12 @@ type Environment interface { | |||||||||||||||||||||
error, | ||||||||||||||||||||||
) | ||||||||||||||||||||||
|
||||||||||||||||||||||
// GetCurrentVersionBoundary executes the getCurrentVersionBoundary function on the NodeVersionBeacon contract. | ||||||||||||||||||||||
// the function will return the version boundary (version, block height) that is currently in effect. | ||||||||||||||||||||||
// the version boundary currently in effect is the highest one not above the current block height. | ||||||||||||||||||||||
// if there is no existing version boundary lower than the current block height, the function will return version 0 and block height 0. | ||||||||||||||||||||||
GetCurrentVersionBoundary() (cadence.Value, error) | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment, but this is a different function. This one just exposes the one on |
||||||||||||||||||||||
|
||||||||||||||||||||||
// AccountInfo | ||||||||||||||||||||||
GetAccount(address flow.Address) (*flow.Account, error) | ||||||||||||||||||||||
GetAccountKeys(address flow.Address) ([]flow.AccountPublicKey, error) | ||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,67 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
package environment | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"github.com/coreos/go-semver/semver" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"github.com/onflow/flow-go/fvm/storage/state" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// MinimumRequiredVersion returns the minimum required cadence version for the current environment | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// in semver format. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
type MinimumRequiredVersion interface { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
MinimumRequiredVersion() (string, error) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be more specific?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this one is a bit tricky. This method satisfies the cadence runtime interface (defined by cadence) and from a cadence perspective it does not make sense to name it a I'll add a comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Maybe we keep the method name as |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
type minimumRequiredVersion struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
txnPreparer state.NestedTransactionPreparer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func NewMinimumRequiredVersion( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
txnPreparer state.NestedTransactionPreparer, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) MinimumRequiredVersion { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return minimumRequiredVersion{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
txnPreparer: txnPreparer, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func (c minimumRequiredVersion) MinimumRequiredVersion() (string, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
executionParameters := c.txnPreparer.ExecutionParameters() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// map the minimum required flow-go version to a minimum required cadence version | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
cadenceVersion := mapToCadenceVersion(executionParameters.ExecutionVersion, minimumFvmToMinimumCadenceVersionMapping) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return cadenceVersion.String(), nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func mapToCadenceVersion(flowGoVersion semver.Version, versionMapping FlowGoToCadenceVersionMapping) semver.Version { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if versionGreaterThanOrEqualTo(flowGoVersion, versionMapping.FlowGoVersion) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return versionMapping.CadenceVersion | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return semver.Version{} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func versionGreaterThanOrEqualTo(version semver.Version, other semver.Version) bool { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return version.Compare(other) >= 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
type FlowGoToCadenceVersionMapping struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
FlowGoVersion semver.Version | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
CadenceVersion semver.Version | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// This could also be a map, but ist not needed because we only expect one entry at a give time | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// we won't be fixing 2 separate issues at 2 separate version with one deploy. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
var minimumFvmToMinimumCadenceVersionMapping = FlowGoToCadenceVersionMapping{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Leaving this example in, so it's easier to understand | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// FlowGoVersion: *semver.New("0.37.0"), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// CadenceVersion: *semver.New("1.0.0"), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
func SetFVMToCadenceVersionMappingForTestingOnly(mapping FlowGoToCadenceVersionMapping) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
minimumFvmToMinimumCadenceVersionMapping = mapping | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
var _ MinimumRequiredVersion = (*minimumRequiredVersion)(nil) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
package environment | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/coreos/go-semver/semver" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func Test_MapToCadenceVersion(t *testing.T) { | ||
flowV0 := semver.Version{} | ||
cadenceV0 := semver.Version{} | ||
flowV1 := semver.Version{ | ||
Major: 0, | ||
Minor: 37, | ||
Patch: 0, | ||
} | ||
cadenceV1 := semver.Version{ | ||
Major: 1, | ||
Minor: 0, | ||
Patch: 0, | ||
} | ||
|
||
mapping := FlowGoToCadenceVersionMapping{ | ||
FlowGoVersion: flowV1, | ||
CadenceVersion: cadenceV1, | ||
} | ||
|
||
t.Run("no mapping, v0", func(t *testing.T) { | ||
version := mapToCadenceVersion(flowV0, FlowGoToCadenceVersionMapping{}) | ||
|
||
require.Equal(t, cadenceV0, version) | ||
}) | ||
|
||
t.Run("v0", func(t *testing.T) { | ||
version := mapToCadenceVersion(flowV0, mapping) | ||
|
||
require.Equal(t, semver.Version{}, version) | ||
}) | ||
t.Run("v1 - delta", func(t *testing.T) { | ||
|
||
v := flowV1 | ||
v.Patch -= 1 | ||
|
||
version := mapToCadenceVersion(v, mapping) | ||
|
||
require.Equal(t, cadenceV0, version) | ||
}) | ||
t.Run("v1", func(t *testing.T) { | ||
version := mapToCadenceVersion(flowV1, mapping) | ||
|
||
require.Equal(t, cadenceV1, version) | ||
}) | ||
t.Run("v1 + delta", func(t *testing.T) { | ||
|
||
v := flowV1 | ||
v.BumpPatch() | ||
|
||
version := mapToCadenceVersion(v, mapping) | ||
|
||
require.Equal(t, cadenceV1, version) | ||
}) | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Optional:
This introduce unnecessary complexity to the GetMinimumExecutionVersion function.
And create a shortcut for tests. Can we mock the the smart contract call in tests instead?
I saw there are also other WithXXX functions having the same pattern which is to use some flag variables only for testing. IMO, this is a bad practice. We shouldn't have production code skipping some logic (shortcuts) only for tests.
I'm ok for now, but better address this altogether in a separate PR.
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.
For most test this was not used. I only used it for tests that are testing the programs cache loading and resetting. The reason I did that is that an extra contract is loading now, and those tests were off by 1. I think its cleaner for those tests to focus on what they were testing without this interference.
I agree its not the bast pattern, and I will try mocking in a separate PR.
However I am thinking of also using this setting for bootstrapping...