Skip to content
This repository has been archived by the owner on Dec 15, 2023. It is now read-only.

Support specifying chain ID #358

Merged
merged 25 commits into from
Dec 5, 2022
Merged

Support specifying chain ID #358

merged 25 commits into from
Dec 5, 2022

Conversation

mikiw
Copy link
Contributor

@mikiw mikiw commented Nov 30, 2022

Usage related changes

  • User can specify --chain_id argument as "MAINNET" or "TESTNET"

Development related changes

  • DEFAULT_GENERAL_CONFIG is now assignet to build_devnet_general_config() function in tests.

Checklist:

  • Applied formatting - ./scripts/format.sh
  • No linter errors - ./scripts/lint.sh
  • Performed code self-review
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Documented the changes
  • Linked the issues which this PR resolves
  • Updated the tests
  • All tests are passing

@mikiw mikiw marked this pull request as draft November 30, 2022 13:57
@mikiw mikiw linked an issue Nov 30, 2022 that may be closed by this pull request
@mikiw mikiw marked this pull request as ready for review November 30, 2022 16:28
@mikiw mikiw requested a review from FabijanC November 30, 2022 16:40
@mikiw
Copy link
Contributor Author

mikiw commented Dec 1, 2022

Just thinking that maybe adding a test for it would be nice but I'm not sure how to verify this chain id in the test.

I'll try to find out.

Copy link
Collaborator

@FabijanC FabijanC left a comment

Choose a reason for hiding this comment

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

The Updated the tests checkbox shouldn't be checked if no tests were introduced.

I would recommend testing an illegal value (see test_fork_cli.py for reference).

A way to test if MAINNET chain ID is set up correctly is to assert that transactions signed using chain_id=TESTNET fail. Notice that in test/account.py::_get_transaction_hash we have StarknetChainId.TESTNET hardcoded - it should be made configurable. It should be asserted that a tx is accepted when the correct chain id is used for signing.

starknet_devnet/devnet_config.py Outdated Show resolved Hide resolved
starknet_devnet/general_config.py Outdated Show resolved Hide resolved
starknet_devnet/general_config.py Outdated Show resolved Hide resolved
@mikiw mikiw requested a review from FabijanC December 1, 2022 22:04
starknet_devnet/general_config.py Outdated Show resolved Hide resolved
test/test_chain_id_cli_params.py Show resolved Hide resolved
test/test_chain_id_cli_params.py Outdated Show resolved Hide resolved
starknet_devnet/devnet_config.py Outdated Show resolved Hide resolved
starknet_devnet/devnet_config.py Outdated Show resolved Hide resolved
page/docs/guide/run.md Outdated Show resolved Hide resolved
@FabijanC FabijanC merged commit bf1ba8d into master Dec 5, 2022
@FabijanC FabijanC deleted the chain-id branch December 5, 2022 11:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support specifying chain ID
2 participants