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

fix upgrade e2e test #1269

Merged
merged 1 commit into from
Jun 19, 2024
Merged

fix upgrade e2e test #1269

merged 1 commit into from
Jun 19, 2024

Conversation

ermalkaleci
Copy link
Contributor

Pull Request Summary

Update to latest chopsticks and bump deps

@ermalkaleci ermalkaleci added ci This PR/Issue is related to the topic "CI" tests If the PR/issue is related to tests, like xcm-simulator tests, rpc-tests etc. labels Jun 19, 2024
Copy link

Code Coverage

Package Line Rate Branch Rate Health
precompiles/unified-accounts/src 100% 0%
chain-extensions/xvm/src 0% 0%
pallets/dapp-staking-v3/src 90% 0%
pallets/dapp-staking-v3/src/benchmarking 98% 0%
pallets/astar-xcm-benchmarks/src/fungible 100% 0%
pallets/astar-xcm-benchmarks/src 88% 0%
pallets/oracle-benchmarks/src 0% 0%
chain-extensions/types/unified-accounts/src 0% 0%
pallets/xvm/src 54% 0%
precompiles/dapp-staking-v3/src/test 0% 0%
chain-extensions/types/assets/src 0% 0%
pallets/ethereum-checked/src 79% 0%
pallets/unified-accounts/src 86% 0%
pallets/price-aggregator/src 72% 0%
pallets/static-price-provider/src 52% 0%
pallets/inflation/src 83% 0%
precompiles/assets-erc20/src 81% 0%
chain-extensions/unified-accounts/src 0% 0%
pallets/dapp-staking-v3/src/test 0% 0%
precompiles/sr25519/src 64% 0%
precompiles/xcm/src 73% 0%
primitives/src 62% 0%
pallets/xc-asset-config/src 64% 0%
pallets/dapp-staking-migration/src 0% 0%
precompiles/dapp-staking-v3/src 90% 0%
precompiles/dispatch-lockdrop/src 86% 0%
precompiles/substrate-ecdsa/src 74% 0%
precompiles/xvm/src 75% 0%
primitives/src/xcm 64% 0%
chain-extensions/pallet-assets/src 56% 0%
pallets/astar-xcm-benchmarks/src/generic 100% 0%
pallets/dapp-staking-v3/rpc/runtime-api/src 0% 0%
pallets/collator-selection/src 92% 0%
pallets/dynamic-evm-base-fee/src 92% 0%
chain-extensions/types/xvm/src 0% 0%
Summary 77% (3644 / 4714) 0% (0 / 0)

Minimum allowed line rate is 50%

Comment on lines +81 to +82
// wait a bit for pjs/api to update
await new Promise((r) => setTimeout(r, 1000));
Copy link
Member

@Dinonard Dinonard Jun 19, 2024

Choose a reason for hiding this comment

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

How come this is needed if you await for the block production above?
Doesn't it mean the newBlock function returns prematurely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not for block production but for ApiPromise to become aware of the new block and storage changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alice is sending both tx and some how it needed a bit of time to sync nonce

Copy link
Member

Choose a reason for hiding this comment

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

Just to check if I understand correctly - remark is executed, followed by setCode, two new blocks are produced, then we wait for the internal chopsticks state to stabilize/sync, before proceeding forward?

I don't know how chopsticks works internally, but should it return from the newBlock if state isn't synced yet? Maybe it's a dumb question 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

once block is build it will resolve and announce storage changes async for listeners (like ApiPromise). But because we're immediately creating a new tx (aslo because everything is running on a single js thread event loop) nextNounce may not be reflected immediately so second tx was build with old nonce resulting in invalid tx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

even a 200ms will be enough but I added 1sec to be sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This appears to happen only when we perform an upgrade, this is making me think why 🤔, need to debug it

Copy link
Contributor Author

@ermalkaleci ermalkaleci Jun 19, 2024

Choose a reason for hiding this comment

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

@Dinonard Initially I thought it was sending old nonce but it's not nonce, it's spec_version. When runtime upgrades happen, it takes a few millis for ApiPromise to receive metadata from chopsticks (because it extracts it from runtime itself, runtime call needs to happen) but pjs/api doesn't wait for the new metadata but instead uses what it has (old spec_version) and making an invalid tx

@ermalkaleci ermalkaleci merged commit e790efd into master Jun 19, 2024
9 of 10 checks passed
@ermalkaleci ermalkaleci deleted the fix/update-upgrade-test branch June 19, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci This PR/Issue is related to the topic "CI" tests If the PR/issue is related to tests, like xcm-simulator tests, rpc-tests etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants