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

feat(chain): add chain serialization #164

Open
wants to merge 3 commits into
base: zygis/harden-docker-networking
Choose a base branch
from

Conversation

Zygimantass
Copy link
Contributor

No description provided.

@Zygimantass Zygimantass force-pushed the zygis/chain-serialization branch from 38b3d17 to bd08ed6 Compare January 15, 2025 12:29
@nadim-az nadim-az changed the base branch from release/v3.x.x to zygis/harden-docker-networking January 16, 2025 13:54
NodeOptions NodeOptions // NodeOptions is the options for creating a node
NodeCreator NodeCreator // NodeCreator is a function that creates a node

WalletConfig WalletConfig // WalletConfig is the default configuration of a chain's wallet
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we mark the WalletConfig option as required and validate its not nil during config validation? During my DO provider testing, I ran into an error when I tried to create a chain without WalletConfig set, but we currently dont do any validation for it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or alternatively add default config

@@ -20,28 +23,31 @@ const idAlphabet = "abcdefghijklqmnoqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ12345678
var defaultChainConfig = types.ChainConfig{
Denom: "stake",
Decimals: 6,
NumValidators: 1,
NumValidators: 4,
NumNodes: 0,
BinaryName: "/simd/simd",
Image: provider.ImageDefinition{
Image: "cosmossdk/simd:latest",
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this image publicly available or just cached on your machine? im not able to pull this or find it in the docker hub

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good shout, will replace it

@@ -121,9 +125,5 @@ func (c *ChainConfig) ValidateBasic() error {
return fmt.Errorf("chain ID cannot be empty")
}

if c.NodeCreator == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this removed? this option is still required afaiu

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to opts

Copy link
Collaborator

Choose a reason for hiding this comment

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

should validation be added elsewhere then for the required opts fields?

@aljo242
Copy link
Contributor

aljo242 commented Jan 16, 2025

Are all of the options we provide to these functions actually optional?

A problem we ran into with connect is that we started falling in love with this options pattern, and would start including options that are actually required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants