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: pass contract_name to VyperContract #338

Merged

Conversation

PatrickAlphaC
Copy link
Contributor

@PatrickAlphaC PatrickAlphaC commented Oct 21, 2024

What I did

add contract_name to VyperContract ctor. allows setting contract_name at deploy time.

How I did it

How to verify it

pytest tests/integration/network/anvil -k test_deployment_db

Description for the changelog

  • Added nickname to contracts

Cute Animal Picture

image

Copy link
Collaborator

@DanielSchiavini DanielSchiavini left a comment

Choose a reason for hiding this comment

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

What's the difference between nickname and contract_name? Do we really need 2 names?

@PatrickAlphaC
Copy link
Contributor Author

What's the difference between nickname and contract_name? Do we really need 2 names?

The contract_name is the name of the actual file, IE ERC20. The nickname is any name you give it, ie DAI. So for example, in the database you could have:

ERC20: DAI
ERC20: USDC
ERC20: BEAN

etc

@PatrickAlphaC
Copy link
Contributor Author

PatrickAlphaC commented Oct 22, 2024

Update:

removed nickname and just made contract_name a parameter. If not given, defatuls to the Path(contract_path).stem like it previously did

@@ -212,6 +212,7 @@ def deploy(
override_address: Optional[_AddressType] = None,
# the calling vyper contract
contract: Any = None,
contract_name: Optional[str] = None, # TODO: This isn't used
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably don't need this, we can call contract.contract_name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone passes contract_name = xxx to the deploy of a non NetworkEnv boa.env, this would revert. So I think we do need this to make it compatible with the NetworkEnv object

Copy link
Member

Choose a reason for hiding this comment

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

i think it would be an error to pass contract_name = xxx to env.deploy().

boa/network.py Outdated
@@ -402,7 +409,11 @@ def deploy(
print(f"contract deployed at {create_address}")

if (deployments_db := get_deployments_db()) is not None:
contract_name = getattr(contract, "contract_name", None)
contract_name = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

why would we want 2 names for the contract? This value contract_name should be equal to contract.contract_name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? This would only set 1 name. It defaults to the filename unless one is passed.

Copy link
Member

Choose a reason for hiding this comment

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

yea i don't think contract_name should be passed to env.deploy(). it already exists on the contract object

Comment on lines 151 to 153
contract_name = (
contract_name if contract_name else Path(compiler_data.contract_path).stem
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
contract_name = (
contract_name if contract_name else Path(compiler_data.contract_path).stem
)
if contract_name is None:
contract_name = Path(compiler_data.contract_path).stem)

@@ -146,8 +146,11 @@ def __init__(
compiler_data: CompilerData,
env: Optional[Env] = None,
filename: Optional[str] = None,
contract_name: Optional[str] = None,
Copy link
Member

Choose a reason for hiding this comment

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

we should follow the same order as the super class tbh

some other style things
move around arg order
@charles-cooper charles-cooper changed the title Feat/nicknamed deployment fix: pass contract_name to VyperContract Oct 23, 2024
@charles-cooper charles-cooper merged commit f58c33c into vyperlang:master Oct 25, 2024
7 of 9 checks passed
Leminkay pushed a commit to Leminkay/titanoboa that referenced this pull request Oct 28, 2024
add contract_name to VyperContract ctor. allows setting contract_name at deploy time.

---------

Co-authored-by: Charles Cooper <[email protected]>
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