tmpnet: Move tmpnet/local to tmpnet package
#2457
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marun
force-pushed
the
tmpnet-move-local
branch
3 times, most recently
from
62916a1 to
5bb8d41
Compare
abi87
reviewed
Dec 10, 2023
abi87
reviewed
Dec 10, 2023
tests/fixture/tmpnet/config.go
Outdated
| @@ -206,6 +197,24 @@ func (nc *NodeConfig) EnsureKeys() error { | |||
| return nc.EnsureNodeID() | |||
| } | |||
|
|
|||
| // Derives the nodes proof-of-possession. Requires the node to have a | |||
| // BLS signing key. | |||
| func (nc *NodeConfig) GetProofOfPosession() (*signer.ProofOfPossession, error) { | |||
There was a problem hiding this comment.
Whoops, fixed in parent.
abi87
reviewed
Dec 10, 2023
tests/fixture/tmpnet/config.go
Outdated
| errInvalidKeypair = fmt.Errorf("%q and %q must be provided together or not at all", config.StakingTLSKeyContentKey, config.StakingCertContentKey) | ||
| errNoKeysForGenesis = errors.New("failed to generate genesis: no keys to fund") | ||
| errInvalidNetworkIDForGenesis = errors.New("network ID can't be mainnet, testnet or local network ID") | ||
| errMissingStakersForGenesis = errors.New("no genesis stakers provided") |
There was a problem hiding this comment.
q: do we set in genesis stakers that are not validators?
There was a problem hiding this comment.
No, I'm assuming that stakers are always validators. Is that a valid assumption?
abi87
reviewed
Dec 10, 2023
abi87
reviewed
Dec 10, 2023
| // Defines the configuration required for a tempoary network | ||
| type Network struct { | ||
| NetworkConfig | ||
| NodeRuntimeConfig |
There was a problem hiding this comment.
q: is is the same runtime config for every node in the network?
There was a problem hiding this comment.
Yes, that is the case. While it was possible to modify the exec path per node (e.g. for the upgrade test) it wasn't persisted. The next PR will enable per-node persistent runtime configuration.
marun
force-pushed
the
tmpnet-move-local
branch
from
5bb8d41 to
0e6d8ec
Compare
marun
changed the title
marun
force-pushed
the
tmpnet-move-local
branch
2 times, most recently
from
f9aada9 to
3e42f16
Compare
marun
force-pushed
the
funded-to-prefunded
branch
from
43e53fc to
437bcb6
Compare
marun
force-pushed
the
tmpnet-move-local
branch
from
3e42f16 to
184b8d8
Compare
marun
changed the title
tmpnet: Move tmpnet/local to tmpnet package
marun
force-pushed
the
tmpnet-move-local
branch
from
184b8d8 to
ae08b7e
Compare
marun
force-pushed
the
tmpnet-move-local
branch
from
ae08b7e to
19dca4a
Compare
aaronbuchwald
approved these changes
Dec 13, 2023
tests/e2e/c/dynamic_fees.go
Outdated
| nodeURI := tmpnet.NodeURI{ | ||
| NodeID: node.GetID(), | ||
| URI: node.GetProcessContext().URI, | ||
| ID: node.ID, |
There was a problem hiding this comment.
optional nit: I think using NodeID is more explicit than ID, which can be confusing at times, so I'd prefer to keep the field name NodeID here
There was a problem hiding this comment.
Ack, I'll make that change.
There was a problem hiding this comment.
Is your comment just for NodeURI, or should it also apply to tmpnet.Node.ID?
There was a problem hiding this comment.
I think we should use the NodeID type for all Node IDs.
marun
force-pushed
the
tmpnet-move-local
branch
from
5552748 to
e20e8f7
Compare
marun
force-pushed
the
tmpnet-move-local
branch
from
e20e8f7 to
93dc814
Compare
darioush
approved these changes
Dec 14, 2023
This is the first step in a refactor in support of deploying temporary networks to kubernetes.
Co-authored-by: aaronbuchwald <[email protected]>
marun
force-pushed
the
tmpnet-move-local
branch
from
4b22d47 to
cb968e8
Compare
6 tasks
marun
added a commit
that referenced
this pull request
Dec 20, 2023
Co-authored-by: aaronbuchwald <[email protected]>
marun
added a commit
that referenced
this pull request
Dec 21, 2023
Co-authored-by: aaronbuchwald <[email protected]>
marun
added a commit
that referenced
this pull request
Dec 22, 2023
Co-authored-by: aaronbuchwald <[email protected]>
marun
added a commit
that referenced
this pull request
Dec 22, 2023
Co-authored-by: aaronbuchwald <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously the tmpnet code was separated into
tmpnetandtmpnet.localpackages. The intention was fortmpnetto define common interfaces and configuration, andtmpnet.localto orchestrate process-based temporary networks.I eventually realized that the previous abstractions were unnecessary. A process- or kube-based network need only differ in its runtime - the code that starts/stops/etc the node. The configuration will be largely similar and can all be stored in the same way. Hence this PR: removing the concept of
Local{Node,Network,Config}in favor of Node/Network/Config. Subsequent PRs will introduce the concept ofNodeRuntimeto support the choice of nodes run as processes or pods.