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

feat: allow empty to field in eth_call to simulate contract deploy #544

Closed
wants to merge 1 commit into from

Conversation

CedarMist
Copy link
Member

Closes #543

@CedarMist CedarMist force-pushed the CedarMist/eth_call-deployment branch from 69b3054 to fe440e0 Compare March 25, 2024 15:37
Copy link
Member

@ptrus ptrus left a comment

Choose a reason for hiding this comment

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

Maybe prefix the commit with fix: so that this ends up as a bugfix in the changelog

@@ -371,7 +371,8 @@ func (api *publicAPI) Call(ctx context.Context, args utils.TransactionArgs, bloc
)

if args.To == nil {
return []byte{}, errors.New("to address not specified")
// When simulating a contract deploy the To address may not be specified
args.To = &common.Address{}
Copy link
Member

@ptrus ptrus Mar 25, 2024

Choose a reason for hiding this comment

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

So this sets the addres to 0x0000000000000000000000000000000000000000, i think this should be nil. Does the runtime fail with nil to address? Should the runtime handle it?

Copy link
Member Author

@CedarMist CedarMist Mar 25, 2024

Choose a reason for hiding this comment

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

When I set To to nil I get:

  error: { code: -32603, message: 'method handler crashed' }

Whereas setting it to all zeros the call succeeds.

I'm having another look at eth_call in runtime SDK, it's looking like we might need a separate codepath to handle creation.

Copy link
Member

@ptrus ptrus Mar 25, 2024

Choose a reason for hiding this comment

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

I think it should be fine to set it to 0x000... here for now, since fixing (and deploying) it in runtime will take some time in any case 👍 Maybe just open a clickup issue to check if this should be addressed in oasis-sdk.

@@ -371,7 +371,8 @@ func (api *publicAPI) Call(ctx context.Context, args utils.TransactionArgs, bloc
)

if args.To == nil {
return []byte{}, errors.New("to address not specified")
// When simulating a contract deploy the To address may not be specified
Copy link
Member

Choose a reason for hiding this comment

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

Also, this should only be allowed if len(args.data()) != 0 (this is a contract deploy).

@CedarMist
Copy link
Member Author

This workaround is not good.

@CedarMist CedarMist closed this Mar 25, 2024
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.

Hardhat-ignition fails to deploy contracts
3 participants