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

Add genesis config option for Node #4017

Merged
merged 5 commits into from
Jan 2, 2025
Merged

Conversation

limebell
Copy link
Member

@limebell limebell commented Dec 30, 2024

This provides genesis base state configuration option.

Use "Genesis:GenesisConfigurationPath" and pass JSON represented base genesis state.

The structure of the JSON file is:

{
    "<AccountAddress>":
    {
        "<Address>":"<Serialized IValue as Hex string>"
    }
}

For more detail, please take a look at sdk/node/Libplanet.Node.Tests/Services/BlockChainServiceTest.cs

@limebell limebell requested a review from s2quake December 30, 2024 11:39
@limebell limebell self-assigned this Dec 30, 2024
@limebell limebell requested a review from riemannulus December 30, 2024 11:40
@limebell limebell force-pushed the node/genesis-config branch from 439d3da to 557e951 Compare December 30, 2024 12:06
@limebell limebell marked this pull request as ready for review December 30, 2024 12:07
}
finally
{
if (File.Exists(tempFilePath))
Copy link
Contributor

Choose a reason for hiding this comment

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

The indent level can be reduced by one if the following code is used.
https://github.com/s2quake/libplanet-console/blob/main/src/common/LibplanetConsole.Common/IO/TempFile.cs
However, it's unfortunate that the code is not available in libplanet.node

[Description(
$"The path of the genesis configuration, which can be a file path or a URI." +
$"This property cannot be used with {nameof(GenesisKey)} and {nameof(GenesisBlockPath)}.")]
public string GenesisConfigurationPath { get; set; } = string.Empty;
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are many ways to create Genesis blocks, it may be better to create a type variable rather than increase the properties.
If the properties increase, the complexity will increase because the exception handling code must be implemented.
For example,

enum GenesisBlockType
{
    Binary,
    BinaryUrl,
    Json,
    JsonUrl
}

I don't know what the best way is, but I think it's worth considering.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this suggestion should be dealt in one separated PR

@@ -8,7 +8,7 @@
<ItemGroup>
<PackageReference Include="Cocona.Lite" Version="2.0.*" />
<PackageReference Include="Serilog.Sinks.Console" Version="4.1.0" />
<PackageReference Include="System.Text.Json" Version="7.0.0" />
<PackageReference Include="System.Text.Json" Version="9.0.*" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Bumping to the latest version is always welcome. At least for me. 😀

Copy link
Member Author

Choose a reason for hiding this comment

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

Vulnerability in 7.0 causes failure in build 😢

@limebell limebell changed the title WIP) Add genesis config option for Node Add genesis config option for Node Jan 2, 2025
@riemannulus riemannulus merged commit 956025f into exp/sdk/node Jan 2, 2025
24 of 29 checks passed
@riemannulus riemannulus deleted the node/genesis-config branch January 2, 2025 06:10
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