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.

Sign up for GitHub

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

Jump to bottom

feat(core): Support deploying stateless non-proxied contracts #580

Merged
merged 2 commits into from
Apr 4, 2023
Merged

feat(core): Support deploying stateless non-proxied contracts #580

merged 2 commits into from
Apr 4, 2023

Conversation

RPate97
Copy link
Collaborator

@RPate97 RPate97 commented Mar 29, 2023 •
edited
Loading

Purpose

  • Adds support for deploying non-proxied contracts as long as they are stateless.
  • Fixes a bug where contract references only work as long as the referenced contract is declared before the contract reference. You can now use contract references out of order.

@mergify
Copy link
Contributor

mergify bot commented Mar 29, 2023

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

@RPate97 RPate97 force-pushed the pate/chu-171-output-all-validation-errors-at-once branch from 30199f9 to 0c1e875 Compare March 30, 2023 02:03
@RPate97 RPate97 force-pushed the pate/chu-147-deploy-stateless-non-proxied-contracts branch 2 times, most recently from af51bf0 to 086a59a Compare March 30, 2023 16:44
@RPate97 RPate97 force-pushed the pate/chu-171-output-all-validation-errors-at-once branch from 0c1e875 to 508fa50 Compare March 30, 2023 16:54
@mergify
Copy link
Contributor

mergify bot commented Mar 30, 2023

Hey @RPate97! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Mar 30, 2023
@RPate97 RPate97 force-pushed the pate/chu-147-deploy-stateless-non-proxied-contracts branch from 086a59a to d92bd05 Compare March 30, 2023 17:22
@mergify mergify bot removed the conflict label Mar 30, 2023
@RPate97 RPate97 force-pushed the pate/chu-147-deploy-stateless-non-proxied-contracts branch 3 times, most recently from bff091a to bafaf34 Compare March 30, 2023 18:03
@RPate97 RPate97 requested review from sam-goldman and removed request for sam-goldman March 30, 2023 18:05
@RPate97 RPate97 force-pushed the pate/chu-147-deploy-stateless-non-proxied-contracts branch from bafaf34 to ed670ea Compare March 30, 2023 19:19
@RPate97 RPate97 mentioned this pull request Mar 30, 2023
Merged
@RPate97 RPate97 force-pushed the pate/chu-171-output-all-validation-errors-at-once branch from 508fa50 to 4608817 Compare March 31, 2023 17:36
@mergify
Copy link
Contributor

mergify bot commented Mar 31, 2023

Hey @RPate97! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Mar 31, 2023
@RPate97 RPate97 force-pushed the pate/chu-147-deploy-stateless-non-proxied-contracts branch from ed670ea to bff7b7b Compare March 31, 2023 17:52
@mergify mergify bot removed the conflict label Mar 31, 2023
sam-goldman
sam-goldman reviewed Mar 31, 2023
View reviewed changes
packages/core/src/config/parse.ts Show resolved Hide resolved
packages/core/src/config/parse.ts Outdated Show resolved Hide resolved
packages/core/src/config/parse.ts Outdated Show resolved Hide resolved
packages/core/src/config/parse.ts Outdated Show resolved Hide resolved
packages/plugins/src/hardhat/deployments.ts Outdated Show resolved Hide resolved
packages/plugins/src/hardhat/deployments.ts
userConfig.options.projectName,
creationCodeWithConstructorArgs
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO it's slightly cleaner to only assign the value of address once instead of re-assigning it if we're dealing with an unproxied contract

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI it looks like this wasn't refactored, but we can just simplify this later

packages/core/src/config/parse.ts Show resolved Hide resolved
packages/core/src/config/parse.ts Outdated Show resolved Hide resolved
packages/core/src/languages/solidity/types.ts Outdated Show resolved Hide resolved
packages/core/src/config/parse.ts Outdated Show resolved Hide resolved
@RPate97 RPate97 force-pushed the pate/chu-171-output-all-validation-errors-at-once branch from 4608817 to 5334700 Compare March 31, 2023 20:50
@mergify
Copy link
Contributor

mergify bot commented Mar 31, 2023

Hey @RPate97! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Mar 31, 2023
@RPate97 RPate97 force-pushed the pate/chu-147-deploy-stateless-non-proxied-contracts branch from bff7b7b to a7f97ef Compare March 31, 2023 20:52
@mergify mergify bot removed the conflict label Mar 31, 2023
Base automatically changed from pate/chu-171-output-all-validation-errors-at-once to develop April 1, 2023 00:48
@RPate97 RPate97 force-pushed the pate/chu-147-deploy-stateless-non-proxied-contracts branch 2 times, most recently from 90d6268 to 2c0001f Compare April 1, 2023 23:41
@RPate97 RPate97 force-pushed the pate/chu-147-deploy-stateless-non-proxied-contracts branch 7 times, most recently from 2be0b65 to dd4ff18 Compare April 3, 2023 23:13
@RPate97 RPate97 requested a review from sam-goldman April 3, 2023 23:20
@RPate97 RPate97 removed the blocked label Apr 3, 2023
@RPate97 RPate97 force-pushed the pate/chu-147-deploy-stateless-non-proxied-contracts branch from dd4ff18 to 3d9f9c2 Compare April 4, 2023 00:40
sam-goldman
sam-goldman reviewed Apr 4, 2023
View reviewed changes
packages/core/src/utils.ts
| {
integration: Integration
artifactPaths: ArtifactPaths
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can simplify this by adding a creationCodeWithConstructorArgs field on the CRE. We can save that for a future PR though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ya know, I thought about doing this, but IMO, I don't think we use this value enough for it to be worth adding to the CRE. I'm open to it though if there are other things it would simplify.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We call getContractAddress ~7 times in our codebase, so I think it'd be worth it. It'd make this function less janky IMO

@RPate97 RPate97 merged commit 8a3cf9b into develop Apr 4, 2023
@RPate97 RPate97 deleted the pate/chu-147-deploy-stateless-non-proxied-contracts branch April 4, 2023 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sam-goldman sam-goldman sam-goldman approved these changes

Assignees
No one assigned
Labels
contracts core executor plugins
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RPate97 @sam-goldman