search

paritytech / zombienet-sdk

ZombieNet SDK
https://paritytech.github.io/zombienet-sdk/zombienet_sdk/
GNU General Public License v3.0
32 stars 8 forks source link

Providing genesis overrides #173

Closed s0me0ne-unkn0wn closed 7 months ago

s0me0ne-unkn0wn commented 7 months ago

I'm currently fighting two problems with the genesis overrides. First is the path to the overrides themselves:

https://github.com/paritytech/zombienet-sdk/blob/46cac3ad44a27a60dee97f9b9fb261c4bb7056d0/crates/orchestrator/src/generators/chain_spec.rs#L346

I'm not sure why /genesis is concatenated here. Probably, it made sense with previous versions of the chainspec schema. With the current schema, overrides live exactly where the pointer points, that is /genesis/runtimeGenesis/patch. Changing that to be just chain_spec_json.pointer_mut(&pointer) fixes the issue in my case, but I'm not sure about backward compatibility, so I'm filing an issue instead of a PR.

The second is that merge() doesn't really merge. It rather overwrites.

https://github.com/paritytech/zombienet-sdk/blob/46cac3ad44a27a60dee97f9b9fb261c4bb7056d0/crates/orchestrator/src/generators/chain_spec.rs#L665-L668

That is, if you have some balances here in the original spec (and that is an array, not an object), and you provide your own balances in genesis_overrides, those two arrays do not merge. You get just your balances, and the original ones are gone.

pepoviola commented 7 months ago

Hi @s0me0ne-unkn0wn, thanks for your feedback. Yes, the code was made with the old chain-spec schema and needs to be adapted (with backward compatibility). Related to the second issue, maybe the naming of the method is misleading since the behavior is similar to v1 but instead of merge should be called override. Did you see the needs to keep the old balances instead override it? And if we take that path I'm not sure if that will be the desired behavior for other keys that have an array as value.

Can you post the versions and the overrides you want to make to reproduce it locally.

Thanks again for your feedback!!

s0me0ne-unkn0wn commented 7 months ago

Somehow I was sure that the cli version used to really merge balances :thinking:

Anyway, even if I strictly needed to merge them, getting the existing balances from the spec and adding them to my override wouldn't be a big deal. So, thinking about it a little more, it is probably not a problem by itself but a documentation issue. I mean, that would be great to have that merge/override logic described adequately in docs.

pepoviola commented 7 months ago

Somehow I was sure that the cli version used to really merge balances

No, we append the balance for each node but genesis update override the values. Also, we need to update the logic here since in v1 now we allow to add new keys that are not present in the genesis. So, the method behave more like upsert that merge/override.

I checked this line and is a bug, I will fix asap.

if let Some(genesis) = chain_spec_json.pointer_mut(&format!("{}/genesis", pointer))

Thanks!!