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

Deployer says Stack Too Deep #14

Closed
shogochiai opened this issue Mar 21, 2024 · 5 comments · Fixed by #17
Closed

Deployer says Stack Too Deep #14

shogochiai opened this issue Mar 21, 2024 · 5 comments · Fixed by #17

Comments

@shogochiai
Copy link
Contributor

Deployment.s.sol with certain complexity insists it is reached to "Stack too deep".

forge <subcommand> --via-ir evades that error but compilation is gonna be way too slow.

I guess JIT EVM can compress deployment code into LLVM and no stack problem would be shown tho. Let's discuss here.

@kaihiroi
Copy link
Contributor

I find the idea of JIT EVM really fascinating. However, it seems that currently, there's no support for JIT execution of generic contract code, so it might be best to wait for native support in Foundry. In the meantime, keeping method chains short can be a workaround to avoid the Stack Too Deep compile error. This approach might result in some repetitive code, but using:

mc.init("hogehoge");
mc.use(FunctionA);
mc.use(FunctionB);
mc.deploy();

instead of chaining them together like:

mc.init("hogehoge").use(FunctionA).use(FunctionB).deploy();

can be effective.

@shogochiai
Copy link
Contributor Author

shogochiai commented Mar 26, 2024

It sounds good for a bundle, although, could you suggest some ideas on multi-bundle deployment script usecase.

The mc is preserving internal state and cache-related problems might undermine stability of deployment process.

I guess mc.reset(); alike cache cleaner is good for now. Or we can reset state inside mc.deploy(); logic.

@kaihiroi
Copy link
Contributor

I agree that implementing mc.reset() and incorporating a reset within mc.deploy() makes a lot of sense. I'll go ahead and implement those changes.

@shogochiai
Copy link
Contributor Author

Okay, close this issue when you merged that feature.

@kaihiroi
Copy link
Contributor

I've implemented mc.reset(). I also experimented with incorporating a reset within mc.deploy(), but decided against it for now since it would make toProxyAddress(), which is likely used in many use cases, unavailable after deployment. It's possible to preserve just the proxy context, so if needed, please feel free to reopen this issue.

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 a pull request may close this issue.

2 participants