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: docs - fix toml key for forking #189

Merged
merged 3 commits into from
Jan 29, 2025
Merged

Conversation

benber86
Copy link
Contributor

Current docs use is_fork, should be fork

Current docs use `is_fork`, should be `fork`
@s3bc40
Copy link
Contributor

s3bc40 commented Jan 27, 2025

Great finding @benber86!

I have just checked how it was set up, and I think there is confusion between the class property and the parameter to set this value:

def _get_set_active_network_from_cli_and_config(
config: Config,
network: str = None,
url: str = None,
fork: bool | None = None,
account: str | None = None,
password_file_path: Path | None = None,
prompt_live: bool | None = None,
explorer_uri: str | None = None,
explorer_api_key: str | None = None,
db_path: str | None = None,
save_to_db: bool | None = None,
) -> Network:

class Network:
name: str
url: str | None = None
chain_id: int | None = None
is_fork: bool = False
is_zksync: bool = False
default_account_name: str | None = None
unsafe_password_file: Path | None = None
save_abi_path: str | None = None
explorer_uri: str | None = None
explorer_api_key: str | None = None
explorer_type: str | None = None
named_contracts: dict[str, NamedContract] = field(default_factory=dict)
prompt_live: bool = True
save_to_db: bool = True
live_or_staging: bool = True
db_path: str | Path = DB_PATH_LOCAL_DEFAULT
extra_data: dict[str, Any] = field(default_factory=dict)
_network_env: _AnyEnv | None = None

But that means that we'll need to change in other existing docs, since the docs/source/core_concepts/networks/forked_networks.rst is not the only one concerned:

On this doc the param is the right one though named_contracts.rst

@PatrickAlphaC do we have to update there too?

@benber86
Copy link
Contributor Author

Thanks for flagging the extra occurrences in the docs which I'd missed. Should also update the restricted value handle here:

Added all to the PR.

@s3bc40
Copy link
Contributor

s3bc40 commented Jan 28, 2025

Good call on the constants/vars.py 👍

@PatrickAlphaC PatrickAlphaC merged commit c18c2ac into Cyfrin:main Jan 29, 2025
2 checks passed
@PatrickAlphaC
Copy link
Member

Big thanks!

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