Skip to content
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

Refactor chains/manager #2665

Closed
wants to merge 1 commit into from
Closed

Refactor chains/manager #2665

wants to merge 1 commit into from

Conversation

joshua-kim
Copy link
Contributor

Why this should be merged

How this works

How this was tested

@joshua-kim joshua-kim force-pushed the refactor-chain-creator branch 10 times, most recently from e94ac03 to 8287633 Compare January 26, 2024 18:40
@joshua-kim joshua-kim changed the title Refactor chains/manager into platformvm Refactor chains/manager Jan 26, 2024
Copy link
Contributor

@StephenButtolph StephenButtolph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the alias changes could be pulled out into a separate PR and would be an easy win.

ProfileDir string
LogFactory logging.Factory
NodeConfig interface{}
Aliaser ids.Aliaser
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Aliaser ids.Aliaser
ChainAliaser ids.Aliaser

lock sync.RWMutex
ipcs *ipcs.ChainIPCs
log logging.Logger
aliaser ids.Aliaser
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
aliaser ids.Aliaser
chainAliaser ids.AliaserReader

@@ -40,7 +40,8 @@ type Info struct {
validators validators.Manager
myIP ips.DynamicIPPort
networking network.Network
chainManager chains.Manager
aliaser ids.Aliaser
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
aliaser ids.Aliaser
chainAliaser ids.AliaserReader

@@ -85,6 +84,7 @@ var (
// Service defines the API calls that can be made to the platform chain
type Service struct {
vm *VM
aliaser ids.Aliaser
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
aliaser ids.Aliaser
chainAliaser ids.AliaserReader

Comment on lines 97 to 107
//TODO error
_ = e.ChainCreator.CreateChain(
txID,
tx.SubnetID,
tx.GenesisData,
tx.VMID,
tx.FxIDs,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want to error here. Currently we just log an error if creating a chain fails - rather than having the node FATAL... Bubbling the error here would cause the node to FATAL.

Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Copy link

github-actions bot commented Mar 3, 2024

This PR has become stale because it has been open for 30 days with no activity. Adding the lifecycle/frozen label will cause this PR to ignore lifecycle events.

@joshua-kim joshua-kim closed this Mar 5, 2024
@StephenButtolph StephenButtolph deleted the refactor-chain-creator branch July 24, 2024 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants