chore: update internally used chain config #3420
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
@Dhaiwat10 will the differences between the local and ignition chains cause issues here? Consensus parameters version for local is V2 and for ignition (and other configs) it is V1.
|
@petertonysmith94 That's a good point. I'm not really sure which one we are supposed to use. I heard from one of the team members that apparently the local config specified by them hasn't been updated in a long time. I'm not sure why the local is Gonna try a few things and get back with some more context for our sync. |
|
It seems our local test chain config uses settings from the Testnet upgrade number 6 Maybe we should go for either the latest Testnet upgrade (upgrade number 9) or even the lastest Mainet Upgrade Both of them seem to be using the This will require changes only on our side if I am not mistaken, and only on TS types related to test helpers. It seems that even though these versions are being updated on the chain config JSON for main-net and test-net, fuel-core always resolves them to 
|
… into dp/update-chain-config
|
@Torres-ssf can you check the changes I made and if they make sense? |
CodSpeed Performance ReportMerging #3420 will not alter performanceComparing Summary
|
| @@ -104,6 +131,7 @@ function getFinalStateConfigJSON({ stateConfig, chainConfig }: SnapshotConfigs) | |||
| * @param fuelCorePath - the path to the fuel-core binary. (optional, defaults to 'fuel-core') | |||
| * @param loggingEnabled - whether the node should output logs. (optional, defaults to true) | |||
| * @param basePath - the base path to use for the temporary folder. (optional, defaults to os.tmpdir()) | |||
| * @param includeInitialState - whether to initialise the chain with some default initial state. (optional, defaults to false) | |||
There was a problem hiding this comment.
Could this be a breaking change? Before we included state by default, granted the param isn't breaking but the change in expectation is? It may need to be default true with a deprecation notice?
There was a problem hiding this comment.
Yeah..good point. I'll default it to true for now
There was a problem hiding this comment.
Even if the default is true, then for this to be a non-breaking change, would we not need to match the current stateConfig.json file contents?
There was a problem hiding this comment.
IMO as long as there is some state it is fine, but it's a good point 🤔
There was a problem hiding this comment.
We're removing the state and changing the gas costs (which has broken a number of our tests), so we can't be 💯 that these changes won't break tests for end consumers. I don't see the rush for this to be merged, so I would favour holding off until the next breaking changes release window.
There was a problem hiding this comment.
While that's a good point @petertonysmith94, it's worth thinking if we can ever be sure if changing the chain config or removing state from the default config can break something for people who might be using them for tests?
The only way we can really be sure is by providing all the state currently present like you said, and phasing it out via a deprecation notice like Dan mentioned.
Thoughts? @FuelLabs/sdk-ts
There was a problem hiding this comment.
Sorry for the lack of context here, but why are we adding this new flag includeInitialState?
Is this a required change for this PR? Or can we make this change in a different PR?
There was a problem hiding this comment.
This new flag had to be added so that there are no breaking changes in the fuels CLI and create fuels since these tools need at least two wallets to be funded by default.
| @@ -104,6 +131,7 @@ function getFinalStateConfigJSON({ stateConfig, chainConfig }: SnapshotConfigs) | |||
| * @param fuelCorePath - the path to the fuel-core binary. (optional, defaults to 'fuel-core') | |||
| * @param loggingEnabled - whether the node should output logs. (optional, defaults to true) | |||
| * @param basePath - the base path to use for the temporary folder. (optional, defaults to os.tmpdir()) | |||
| * @param includeInitialState - whether to initialise the chain with some default initial state. (optional, defaults to false) | |||
There was a problem hiding this comment.
Even if the default is true, then for this to be a non-breaking change, would we not need to match the current stateConfig.json file contents?
| @@ -121,7 +121,7 @@ const nodeWithCustomBaseAssetId = await launchTestNode({ | |||
| snapshotConfig: { | |||
| chainConfig: { | |||
| consensus_parameters: { | |||
| V1: { | |||
| V2: { | |||
There was a problem hiding this comment.
Is this a breaking change?
There was a problem hiding this comment.
Breaking change for the chain config, not for our SDK consumers. I think.
apps/docs/src/guide/provider/snippets/functionality/get-messages-by-nonce.ts
Outdated
Show resolved
Hide resolved
stateConfig.json#2783Summary
This PR updates our internally used chain config to match the one specified here and removes all the state from our default
stateConfig.jsonfiles.Checklist