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

adding network removed event #1157

Closed
wants to merge 15 commits into from
Closed

Conversation

haider-rs
Copy link
Contributor

Description

closes: #1156

Type of change

  • New feature (non-breaking change which adds functionality)

Tests

  • test_add_and_remove_network

Code review prechecks:

  • Code follows the style guidelines of this project
  • Code has been self-reviewed
  • Inline comments have been added for each method
  • I have made corresponding changes to the documentation
  • Code changes introduces no new problems or warnings
  • Test cases have been added

@haider-rs haider-rs self-assigned this Sep 30, 2024
@haider-rs haider-rs linked an issue Sep 30, 2024 that may be closed by this pull request
@haider-rs haider-rs added the !ci-benchmark Benchmark and commit new weights label Sep 30, 2024
Copy link
Contributor

@penumbra23 penumbra23 left a comment

Choose a reason for hiding this comment

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

Why is the analog-gmp submodule updated in almost every PR?

@haider-rs
Copy link
Contributor Author

@penumbra23 yes thats because testnet branch uses a different version of analog-gmp and main uses a different so everytime we open a PR the analog-gmp branch is old and we forget to revert back to that commit.

@haider-rs haider-rs force-pushed the testnet-network-removed branch from b5cf65a to 5e3514c Compare October 2, 2024 12:35
@FlorianFranzen
Copy link
Collaborator

So what happens with all the other data of that network on chain?

Would that mean if I remove a network and add one back with the same chain id, I would still have all the previous tasks and shards assigned?

@dvc94ch
Copy link
Collaborator

dvc94ch commented Oct 2, 2024

It doesn't really matter since it's going in the Testnet branch. Adeel requested it, so was either discussing why it doesn't make sense for 2h or do it in 10min

@haider-rs haider-rs force-pushed the testnet-network-removed branch from 40e616d to 3afe855 Compare October 3, 2024 13:10
@FlorianFranzen
Copy link
Collaborator

@dvc94ch It does matter very much, because it is going to testnet. Do we for example have tests what happens when tasks are schedule on non-existing networks? Maybe just running try-runtime in follow mode would be good.

Did anybody do any analysis, where network ids are used or iterated over?

@dvc94ch
Copy link
Collaborator

dvc94ch commented Oct 3, 2024

Bran told Adeel it was nonsensical. I wasn't in the meeting. Please discuss the assignment with Adeel...

@haider-rs
Copy link
Contributor Author

Agree with Florian, If the PR made any unexpected storage corruption there is no going back except from complete reset.
cc: @0x1100010010

@dvc94ch dvc94ch closed this Oct 7, 2024
@penumbra23 penumbra23 deleted the testnet-network-removed branch October 31, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
!ci-benchmark Benchmark and commit new weights
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add remove network implementation for testnet.
5 participants