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

Handle polkadot forks #483

Open
wants to merge 56 commits into
base: dev
Choose a base branch
from
Open

Handle polkadot forks #483

wants to merge 56 commits into from

Conversation

rustlang-dev
Copy link
Contributor

@rustlang-dev rustlang-dev commented Sep 17, 2024

Description of proposed changes

Handle polkadot forks
Clean up and refactoring in progress.


Practical tips for PR review & merge:

  • All GitHub Actions report PASS
  • Newly added code/functions have unit tests
    • Coverage tools report all newly added lines as covered
    • The positive scenario is exercised
    • Negative scenarios are exercised, e.g. assert on all possible errors
    • Assert on events triggered if applicable
    • Assert on changes made to storage if applicable
  • Modified behavior/functions - try to make sure above test items are covered
  • Integration tests are added if applicable/needed

rustlang-dev and others added 30 commits September 6, 2024 22:18
use latest polkadot sdk and frontier versions: stable2407
…ade-finished

feat: creditcoin3 runtime polkadot/frontier dependency finished.
feat: update frontier rpc trace to the latest version of polkadot.
feat: upgrade cc3 node to latest frontier, polkadot sdk + todo
fix: run of Subcommand::FrontierDb inside node
Copy link
Contributor

@beqaabu beqaabu left a comment

Choose a reason for hiding this comment

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

looking good so far but needs cleanup

@rustlang-dev rustlang-dev marked this pull request as ready for review September 23, 2024 13:14
Copy link

For full LLVM coverage report click here!

@atodorov
Copy link
Contributor

From integration-tests-cli:

Error: src/lib/staking/bond.ts(69,37): error TS2554: Expected 7 arguments, but got 6.
error Command failed with exit code 2.

This will have an impact on 2 fronts:

  1. Most likely Creditcoin 3 Staking Dashboard will be affected and will need to be updated as well
  2. Older creditcoin-cli clients (e.g. ones which don't upgrade their docker images) won't be able to bond (probably other operations too) after the runtime upgrade occurs. That would generate lots of support calls from the community and some degree of bad publicity so it needs to be communicated well in advance.


type RuntimeFreezeReason = RuntimeFreezeReason;
// type FreezeIdentifier = RuntimeFreezeReason;
// type MaxFreezes = frame_support::traits::VariantCountOf<RuntimeFreezeReason>;
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO remove comments if not in use. Better keep the code base cleaner.

Copy link
Contributor

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Minor issues noted.

However while at it it would be great to rebase this PR in order to solve a conflict in Cargo.toml as well as get rid of all the "Merge " commits currently present.


import-storage:
Sudo:
Key: 5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY # Alice
Copy link
Contributor

Choose a reason for hiding this comment

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

Triggers gitleaks -> https://github.com/gluwa/creditcoin3/actions/runs/11077595872/job/31400820960?pr=483

which seems like a false positive. You can disable it in .gitleaksignore, see the logs for more details.

Note: I think when the commit hash changes .gitleaksignore needs to be updated!

@@ -0,0 +1,17 @@
endpoint: wss://mainnet3.creditcoin.network
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: why not name this file chopsticks.yml or chopsticks-config.yml or something similar that would make it obvious what it is for ?

allow-unresolved-imports: true
mock-signature-host: true
# wasm-override: /Users/mykyta/dev/gluwa/creditcoin3-before-forks-update/target/release/wbuild/creditcoin3-runtime/creditcoin3_runtime.compact.compressed.wasm

Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants