frequency-chain / frequency

Frequency: A Polkadot Parachain
https://www.frequency.xyz
Apache License 2.0
49 stars 18 forks source link

Development Genesis Config has Mainnet Schemas #2057

Closed wilwade closed 2 months ago

wilwade commented 3 months ago

Goal

The goal of this PR is to add the Schemas in at Genesis from Mainnet when on a development chain

Closes #2041

Discussion

How to Test

Checklist

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 56.60377% with 23 lines in your changes missing coverage. Please review.

Files Coverage Δ
common/primitives/src/handles.rs 19.35% <ø> (ø)
pallets/passkey/src/types.rs 68.75% <ø> (ø)
pallets/schemas/src/lib.rs 81.71% <94.44%> (+2.82%) :arrow_up:
pallets/schemas/src/types.rs 87.95% <20.00%> (-4.36%) :arrow_down:
common/primitives/src/schema.rs 58.65% <40.00%> (-0.10%) :arrow_down:
JoeCap08055 commented 2 months ago

I think there's one problem with this: if you start/stop/start a node with the same chainstorage directory, but change the genesis-schemas.json file in between the start/stop, the genesis hash changes and all historical blocks go to hell...

Steps to reproduce:

Do we care about this scenario? Only going to happen in local dev, I guess, but we should have a way to make sure that we don't restart a node with a different genesis config than it was first started with, unless we want to reset the chain...

Thinking this could be an issue when we incorporate this into the Docker image for dev...

wilwade commented 2 months ago

I think there's one problem with this: if you start/stop/start a node with the same chainstorage directory, but change the genesis-schemas.json file in between the start/stop, the genesis hash changes and all historical blocks go to hell...

From my test, this is only due to Polkadotjs. If you reload Polkadotjs/app after restarting the chain, the issues resolve. I've run into this with Polkadotjs/app before.

@JoeCap08055 Can you confirm this fixes/was tried?

Thinking this could be an issue when we incorporate this into the Docker image for dev...

You cannot rebuild the binary, so this at least cannot cause issues outside of the normal restart.

JoeCap08055 commented 2 months ago

I think there's one problem with this: if you start/stop/start a node with the same chainstorage directory, but change the genesis-schemas.json file in between the start/stop, the genesis hash changes and all historical blocks go to hell...

From my test, this is only due to Polkadotjs. If you reload Polkadotjs/app after restarting the chain, the issues resolve. I've run into this with Polkadotjs/app before.

@JoeCap08055 Can you confirm this fixes/was tried?

Thinking this could be an issue when we incorporate this into the Docker image for dev...

You cannot rebuild the binary, so this at least cannot cause issues outside of the normal restart.

Confirmed--reloading the Polkadot.js app/page did the trick. I guess it's really more about proper user education, since it's mainly for dev purposes. We should also be very intentional about how we integrate this into the dsnp/instant-seal-node-with-deployed-schemas image (or, given this, do we even need to maintain 2 separate images any more?)

wilwade commented 2 months ago

@JoeCap08055

or, given this, do we even need to maintain 2 separate images any more

No-ish. For most developers, they would be able to use the Frequency standalone-node image directly.

If someone wanted to have a docker image that would have additional, not-yet-mainnet schemas, then they would need to have a separate image.

So I think we can deprecate that image on docker hub at least.

JoeCap08055 commented 2 months ago

If someone wanted to have a docker image that would have additional, not-yet-mainnet schemas, then they would need to have a separate image.

So I think we can deprecate that image on docker hub at least.

Might be nice to add a fetch target for Paseo schemas as well, to give devs an easy option for that. Beyond that, for testing new, un-deployed schemas, it's easy enough to hand-edit the JSON file with any additional schemas for testing.

wilwade commented 2 months ago

Might be nice to add a fetch target for Paseo schemas as well

I'm actually wondering if we want to force Paseo to also match mainnet. Either by occasionally wiping all schemas or just using setStorage with sudo on Testnet. Effectively, just always have them all match mainnet, but numbers > than mainnet are all yours.

Thoughts?

JoeCap08055 commented 2 months ago

Might be nice to add a fetch target for Paseo schemas as well

I'm actually wondering if we want to force Paseo to also match mainnet. Either by occasionally wiping all schemas or just using setStorage with sudo on Testnet. Effectively, just always have them all match mainnet, but numbers > than mainnet are all yours.

Thoughts?

Seems out of scope for this PR, and worth further discussion, but pretty low priority. Would have been more important before schema names...