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

gas estimation error while deploying smart accounts using default node #347

Closed
sarahschwartz opened this issue Sep 17, 2024 · 4 comments · Fixed by #377
Closed

gas estimation error while deploying smart accounts using default node #347

sarahschwartz opened this issue Sep 17, 2024 · 4 comments · Fixed by #377
Labels
bug 🐛 Something isn't working medium 🚩 Indicates moderately difficult item p0 ⚪ Indicates blocker priority item

Comments

@sarahschwartz
Copy link

sarahschwartz commented Sep 17, 2024

🐛 Bug Report for zkSync Era In-Memory Node

📝 Description

Trying to deploy a smart account to a local node with era_test_node only works by forking the sepolia testnet first when using the latest hardhat plugin. Running the standard node results in an error estimating gas:

Error: execution reverted: Deployment failed (payload={ "id": 3, "jsonrpc": "2.0", "method": "zks_estimateFee", "params": [ { "data": "0x76fb8b6500000000000000000000000000000000000000000000000000000000000000000000000000000000000000007a192369cb8e3c45eabec4750d8673d084e996fc000000000000000000000000a34a855daabd08b7e716bca0ab92ff44e47ad1d7", "from": "0xbc989fde9e54cad2ab4392af6df60f04873a033a", "to": "0x9c1a3d7c98dbf89c7f5d167f2219c29c2fe775a7" } ] }, error={ "code": 3, "data": "0x08c379a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000114465706c6f796d656e74206661696c6564000000000000000000000000000000", "message": "execution reverted: Deployment failed" }

🔄 Reproduction Steps

  1. Clone this repo: https://github.com/dutterbutter/min-custom-aa & install deps
  2. Start a local node with era_test_node run
  3. Add a .env file and add the private key of a pre-configured rich wallet.
  4. Change the default network to inMemoryNode in the hardhat config.
  5. Update the provider url in deploy/deploy-multisig.ts to point to the local node (http://127.0.0.1:8011).
  6. Run bun run compile
  7. Run bun hardhat deploy-zksync --script deploy-factory.ts
  8. Add the deployed factory address to deploy/deploy-multisig.ts
  9. Run bun hardhat deploy-zksync --script deploy-multisig.ts

🤔 Expected Behavior

The script should succeed and deploy a new instance of a smart account using the factory.

😯 Current Behavior

The last script will fail with the error above.

@sarahschwartz sarahschwartz added the bug 🐛 Something isn't working label Sep 17, 2024
@MexicanAce MexicanAce added medium 🚩 Indicates moderately difficult item p0 ⚪ Indicates blocker priority item labels Sep 18, 2024
@dutterbutter
Copy link
Collaborator

@MexicanAce thoughts on making a call to zks_getFeeParams upon start to get the most accurate gas params?

Couple of my own thoughts:

  • gas params on run are always outdated and not as accurate as they should be this would resolve that issue
  • it would require having an internet connection to fetch the params, whereas rn if you have the binary you can simply run and test
  • the unit tests would need to be updated to following the same pattern now that they wouldnt be hardcoded

Let me know your thoughts here!

@MexicanAce
Copy link
Collaborator

That should be fine, so long as we handle the scenario of no internet connection or bad response from the server, and have a backup/default value to use in its place.

That being said, not all the values we need are returned in zks_getFeeParams, but it's still better than just always using hard-coded values.

@MexicanAce
Copy link
Collaborator

Unit tests will mock the response of zks_getFeeParams, so not worried about dynamic values there. The end-to-end tests will need to be watched for downstream flakiness though

@cpb8010
Copy link

cpb8010 commented Nov 4, 2024

So after like 20 failed attempts to update the gas parameters in hopes of fixing this, I'm now thinking this is the same symptom I saw here: https://github.com/matter-labs/zksync-account-sdk/issues/47. Which means this has nothing to do with gas estimation and everything to do with the order of operations for code deployment.

Deploying the account contract before attempting to deploy the factory fixes the issue (for me at least locally). I was never even able to get the deployment to work by forking Sepolia, so it's possible I just have an entirely different setup and issue.
Lyova explains the root cause for my issue as:

Because when we deploy a factory, we also have to supply its factoryDeps -- bytecodes of the contracts that this factory might deploy. If we don't, server will not know the bytecode preimages of the contracts deployed from this factory, and thus won't be able to execute the deploy transaction.

The workaround of deploying the proxy directly before the factory works because this makes proxy's bytecode known in the server, so that when we deploy the factory we don't have to supply additional factoryDeps for that proxy.

https://docs.zksync.io/build/developer-reference/ethereum-differences/contract-deployment#note-on-factory_deps

So to me this says we need to fix our factory deps to actually deploy the contract or at least explain that the bytecode needs to exist before attempting to deploy it within a transaction.
I thought I was going to have a quick fix for this 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working medium 🚩 Indicates moderately difficult item p0 ⚪ Indicates blocker priority item
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants