fix: ETCM-8267 Configure and use the native token addresses in partner-chains-cli #68

AmbientTea · 2024-08-30T20:14:08Z

Description

Generating the chain spec needs three new environmental variables to be configured, after the addition of the native token pallet. This PR modifies prepare-configuration step to also prompt for the addresses and save them, and then uses them in create-chain-spec.

Checklist

Commit sequence broadly makes sense and commits have useful messages.
New tests are added if needed and existing tests are updated.
Relevant logging and metrics added
CI passes. See note on CI.
Any changes are noted in the changelog.md for affected crate
Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG Partner Chains developers to do this
for you.

mpskowron · 2024-09-02T07:11:01Z

partner-chains-cli/src/prepare_configuration/prepare_main_chain_config.rs

@@ -129,6 +132,21 @@ fn run_sidechain_main_cli_addresses<C: IOContext>(
 	Ok(())
 }

+fn prepare_native_token<C: IOContext>(context: &C) -> anyhow::Result<()> {
+	if context.prompt_yes_no("Do you want to configure a native token for you Partner Chain?", true)


I'm missing a better explanation here - what is it for and how to prepare the configuration variables.

I added a slightly more verbose explanation in the wizard.

LGLO · 2024-09-02T12:55:59Z

partner-chains-cli/src/prepare_configuration/prepare_main_chain_config.rs

+	{
+		NATIVE_TOKEN_POLICY.prompt_with_default_from_file_and_save(context);
+		NATIVE_TOKEN_ASSET_NAME.prompt_with_default_from_file_and_save(context);
+		ILLIQUID_SUPPLY_ADDRESS.prompt_with_default_from_file_and_save(context);


IlliquidCirculationSupplyValidator is present in addresses output and should be placed in the part of JSON as the other addresses and policy ids.

Fixed this, now the address comes from the addresses output and placed with other addresses in the chain configuration.

LGLO

In this PR missing env variables are set for chain spec creation, but neither user is lectured about the values nor they are used in setup main chain state phase, so it will make this CLI incomplete and chain not set up properly.

partner-chains-cli/src/config.rs

LGLO · 2024-09-23T06:48:18Z

partner-chains-cli/src/config.rs

 #[derive(Deserialize)]
 pub struct ChainConfig {
 	pub cardano: CardanoParameters,
 	pub chain_parameters: SidechainParams,
 	pub cardano_addresses: MainChainAddresses,
+	pub native_token: NativeTokenConfig,


A: Is it already approved that we support using exactly one ?

B: this is logically bound to illiquid_supply_address from cardano_addresses. I think it is better to keep this asset id where we have other policy ids. And what is more, if not structurally bound like:

struct IlliquidSupplyConfig { asset: AssetConfig, // or assets: Vec[AssetConfig] - depends on decision how many are supported. address: String, }

then at least name here should help to bind these together

A: Yes, we're supporting only one until MN requires us to support multiple. That's decided by @gilligan

B: Okay, I moved it into cardano_addresses under native_token

LGLO · 2024-09-23T06:54:20Z

partner-chains-cli/src/prepare_configuration/prepare_main_chain_config.rs

+	context.print(
+		"Partner Chains can store their initial token supply on Cardano as Cardano native tokens.",
+	);
+	context.print("Creation of the native token is not supported by this wizard and must be performed manually before this step.");


Is there a plan, like in JIRA, to have this supported?
What should be the flow? At what point is policy_id/asset_name chosen? Does it involve using smart-contracts CLI?

I talked with Piotr about this, and he told me we don't want to support this at all, at least for the time being. The issue I see with doing that, is that the token policy can have custom logic in it, so whatever we provide would be relevant only to those PC devs that want a token with exactly the logic that we provide (presumably none).

At what point is policy_id/asset_name chosen? Does it involve using smart-contracts CLI?

No, it can be any native token on Cardano. One can even be created using an existing creator, but someone can roll their own token code with custom logic and deploy it themselves

LGLO · 2024-09-23T13:22:44Z

partner-chains-cli/src/prepare_configuration/prepare_main_chain_config.rs

+		NATIVE_TOKEN_ASSET_NAME.prompt_with_default_from_file_and_save(context);
+	} else {
+		NATIVE_TOKEN_POLICY.save_to_file(&PolicyId::default().to_hex_string(), context);
+		NATIVE_TOKEN_ASSET_NAME.save_to_file(&"0x0".into(), context);


Isn't it a bug? 0x0 doesn't seem to be a syntactically correct hex string. 0x should be empty one, right?

0x0 is a valid hex string, I tested it.

0x is also valid, but to me it looks invalid so I went with 0x0.

LGLO

I cannot even pass generate-keys using this branch.

➜  test-native-token-cli ./partner-chains-cli generate-keys                      
This 🧙 wizard will generate the following keys and save them to your node's keystore:
→  an ECDSA Cross-chain key
→  an ED25519 Grandpa key
→  an SR25519 Aura key
It will also generate a network key for your node if needed.

> node base path ./data

⚙️ Generating Cross-chain (ecdsa) key
running external command: ./partner-chains-node key generate --scheme ecdsa --output-type json
💾 Inserting Cross-chain (ecdsa) key
running external command: ./partner-chains-node key insert --base-path ./data --scheme ecdsa --key-type crch --suri 'grace fence account between allow autumn toilet river river toilet climb repair'
command output: Error: Input("Reading configuration from environment failed: missing value for field native_token_policy_id")
Running executable failed with status exit status: 1

Signed-off-by: Nikolaos Dymitriadis <nikolaos.dymitriadis@iohk.io>

AmbientTea requested review from vvviiimmm, LGLO and mpskowron August 30, 2024 20:15

mpskowron approved these changes Sep 2, 2024

View reviewed changes

LGLO reviewed Sep 2, 2024

View reviewed changes

LGLO requested changes Sep 17, 2024

View reviewed changes

AmbientTea force-pushed the ETCM-8267-native-token-in-cli branch 2 times, most recently from 02d3822 to 3d2f3c7 Compare September 20, 2024 15:05

LGLO self-requested a review September 20, 2024 15:19

AmbientTea changed the base branch from master to better-assert-in-cli-test September 23, 2024 06:10

LGLO reviewed Sep 23, 2024

View reviewed changes

partner-chains-cli/src/config.rs Outdated Show resolved Hide resolved

LGLO reviewed Sep 23, 2024

View reviewed changes

AmbientTea force-pushed the ETCM-8267-native-token-in-cli branch from 3d2f3c7 to c1f7d24 Compare September 23, 2024 10:45

AmbientTea requested review from LGLO, gilligan and m2ux September 23, 2024 11:00

LGLO reviewed Sep 23, 2024

View reviewed changes

feat: rename pallet_session in reference runtime (#92)

04f955a

LGLO requested changes Sep 23, 2024

View reviewed changes

AmbientTea added 10 commits September 23, 2024 17:37

better result and IO assertions in CLI tests

f8dc830

Signed-off-by: Nikolaos Dymitriadis <nikolaos.dymitriadis@iohk.io>

improvements

e6d7997

Signed-off-by: Nikolaos Dymitriadis <nikolaos.dymitriadis@iohk.io>

merge imports

61cd27c

Signed-off-by: Nikolaos Dymitriadis <nikolaos.dymitriadis@iohk.io>

remove assertion macros

2a26772

Signed-off-by: Nikolaos Dymitriadis <nikolaos.dymitriadis@iohk.io>

change msg

063bffb

Signed-off-by: Nikolaos Dymitriadis <nikolaos.dymitriadis@iohk.io>

configuring native token

d390ff7

Signed-off-by: Nikolaos Dymitriadis <nikolaos.dymitriadis@iohk.io>

native token in build chain spec

37e04f1

Signed-off-by: Nikolaos Dymitriadis <nikolaos.dymitriadis@iohk.io>

fix tests

69700dd

Signed-off-by: Nikolaos Dymitriadis <nikolaos.dymitriadis@iohk.io>

changelog

99cf0a6

Signed-off-by: Nikolaos Dymitriadis <nikolaos.dymitriadis@iohk.io>

fix

7e98982

Signed-off-by: Nikolaos Dymitriadis <nikolaos.dymitriadis@iohk.io>

AmbientTea added 3 commits September 23, 2024 17:37

renames

1fcb875

Signed-off-by: Nikolaos Dymitriadis <nikolaos.dymitriadis@iohk.io>

move asset

f7e397c

Signed-off-by: Nikolaos Dymitriadis <nikolaos.dymitriadis@iohk.io>

add dummy vars

6635b7b

AmbientTea marked this pull request as draft September 23, 2024 14:50

AmbientTea removed request for gilligan, vvviiimmm and m2ux September 23, 2024 14:51

AmbientTea force-pushed the ETCM-8267-native-token-in-cli branch from c1f7d24 to 6635b7b Compare September 23, 2024 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ETCM-8267 Configure and use the native token addresses in partner-chains-cli #68

fix: ETCM-8267 Configure and use the native token addresses in partner-chains-cli #68

AmbientTea commented Aug 30, 2024 •

edited

Loading

mpskowron Sep 2, 2024

AmbientTea Sep 20, 2024

LGLO Sep 2, 2024

AmbientTea Sep 20, 2024

LGLO left a comment

LGLO Sep 23, 2024

AmbientTea Sep 23, 2024

LGLO Sep 23, 2024

AmbientTea Sep 23, 2024

AmbientTea Sep 23, 2024

LGLO Sep 23, 2024

AmbientTea Sep 23, 2024

LGLO left a comment

fix: ETCM-8267 Configure and use the native token addresses in partner-chains-cli #68

Are you sure you want to change the base?

fix: ETCM-8267 Configure and use the native token addresses in partner-chains-cli #68

Conversation

AmbientTea commented Aug 30, 2024 • edited Loading

Description

Checklist

Note on CI

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LGLO left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LGLO left a comment

Choose a reason for hiding this comment

AmbientTea commented Aug 30, 2024 •

edited

Loading