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

Test deploy cheatcode #2132

Closed
wants to merge 6 commits into from
Closed

Test deploy cheatcode #2132

wants to merge 6 commits into from

Conversation

Arcticae
Copy link
Member

@Arcticae Arcticae commented Jun 26, 2023

Checklist

  • Relevant issue is linked
  • Docs updated/issue for docs created
  • Added relevant tests

Closes #2108

Note:
Not much more can be tested ATM to be honest

@Arcticae Arcticae marked this pull request as ready for review June 27, 2023 09:09
@Arcticae Arcticae requested a review from a team as a code owner June 27, 2023 09:09
PreparedContract {
class_hash,
constructor_calldata: @calldata,
contract_address: 'addr'
Copy link
Member

Choose a reason for hiding this comment

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

So deploy will use whatever contract address it gets passed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes sir

Copy link
Member Author

Choose a reason for hiding this comment

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

As you can see here

Copy link
Contributor

@MaksymilianDemitraszek MaksymilianDemitraszek Jul 3, 2023

Choose a reason for hiding this comment

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

Would be nice if he had some simplification here so user can write something like this

PreparedContract::from_class_hash(class_hash);

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally user would use deploy_contract for the simplest flow

Copy link
Contributor

@MaksymilianDemitraszek MaksymilianDemitraszek Jul 3, 2023

Choose a reason for hiding this comment

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

Ideally user would use deploy_contract for the simplest flow

I think deploy_contract will be obsolete when we remove declare cheatcode, and predeclare all contracts instead

Copy link
Member Author

Choose a reason for hiding this comment

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

Was not aware of such idea, fair point. Although this is out of scope for this PR.

@piotmag769
Copy link
Contributor

Could we add a task to extend those test when prepare will be ready?

@cptartur
Copy link
Member

Could we add a task to extend those test when prepare will be ready?

You can add a comment to #1991


let contract_address = deploy(
PreparedContract {
class_hash,
Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen if non-existing class_hash is passed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess panic, but this can be tested

let contract_address = deploy(
PreparedContract {
class_hash,
constructor_calldata: @calldata,
Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen if error is raised by the constructor?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure constructor is executed, can we have test case for that?

Copy link
Member Author

@Arcticae Arcticae Jul 3, 2023

Choose a reason for hiding this comment

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

What will happen if error is raised by the constructor

For now it panics, but it's not the target behavior

Are we sure constructor is executed, can we have test case for that?

OK.

@Arcticae
Copy link
Member Author

Arcticae commented Jul 4, 2023

This one is on hold until we figure out a smart way to write tests

@Arcticae Arcticae marked this pull request as draft July 4, 2023 12:49
@Arcticae Arcticae closed this Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contract deploy tests
5 participants