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

Fix Up Estimate Gas #80

Merged
merged 3 commits into from
Aug 28, 2024
Merged

Fix Up Estimate Gas #80

merged 3 commits into from
Aug 28, 2024

Conversation

rauljordan
Copy link
Contributor

Description

Closes #72
Closes #76

Ensures that estimate gas does not error, and shows deployment estimates in ETH and gas price for both deploy and activate txs

@emnul
Copy link
Contributor

emnul commented Aug 20, 2024

Hi @rauljordan I'm trying to test out this PR locally, but I'm running into some issues. I've uninstalled the cargo-stylus and cargo-stylus-check binaries and ran the install.sh script at the project root to reinstall the binaries from my local clone of the repo that's on the fix-estimate-gas feature branch .

git log output confirms I'm up to date on the feature branch

commit d2851f3f901081fc977b2e3932f39d4df67085bc (HEAD -> fix-estimate-gas, origin/fix-estimate-gas)
Author: Raul Jordan <[email protected]>
Date:   Fri Aug 16 10:17:49 2024 -0500

    edits

Unfortunately, it looks like I'm still using the cargo-stylus version that's on the main branch.

Any idea what the issue might be?

@emnul
Copy link
Contributor

emnul commented Aug 20, 2024

Issue had nothing to do with my local version, that was all configured correctly. I glossed over this piece of output when I was testing:

Running in a Docker container for reproducibility, this may take a while
NOTE: You can opt out by doing --no-verify
Running reproducible Stylus command with toolchain 1.79.0

Testing with the --no-verify flag allowed me to successfully test this feature via my local repo.

@gligneul gligneul self-requested a review August 27, 2024 14:51
Copy link
Contributor

@gligneul gligneul left a comment

Choose a reason for hiding this comment

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

Looks good! I tested here and it works properly. One thing that bothers me a little is that these estimate commands require a private key, even though they don't use it. Maybe we can refactor the estimates into a separate command?

@rauljordan
Copy link
Contributor Author

Thanks @gligneul that might be a bit more of an involved refactor, so I'll take care of it in a separate PR

@rauljordan rauljordan merged commit 6fa313d into main Aug 28, 2024
7 checks passed
@rauljordan rauljordan deleted the fix-estimate-gas branch August 28, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--estimate-gas flag causes cargo stylus to fail [Feature Suggestion] Show cost of deployment in Eth
3 participants