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 for Zero knowledge precompile #151

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

luckychacha
Copy link
Contributor

No description provided.

Cargo.toml Outdated
@@ -161,6 +161,7 @@ fc-rpc-core = { version = "1.1.0-dev", git = "https://github.com/AstarNetwork/fr

# Frontier Primitive
fc-storage = { version = "1.0.0-dev", git = "https://github.com/AstarNetwork/frontier.git", branch = "polkadot-v0.9.40", default-features = false }
fp-account = { version = "1.0.0-dev", git = "https://github.com/AstarNetwork/frontier.git", branch = "polkadot-v0.9.40", default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

@luckychacha Why do we enclude this from the outside repo. Can we for it maybe? @xcthulhu What do you think? Otherwise we will have a potential security hole if our installation depends on third part githubs

Copy link
Contributor Author

@luckychacha luckychacha Aug 21, 2023

Choose a reason for hiding this comment

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

  1. The dependency fp-account in my requirements is used to build the mock runtime for benchmark's unit tests, and thus is placed under [dev-dependencies] in the runtime-common module. It will not be included in the release version. Moreover, I have found alternative ways to build the mock runtime, so I will remove the fp-account.workspace = true entry from the runtime-common's Cargo.toml file under [dev-dependencies].
  2. I have noticed that our cargo.toml file contains a patch for this dependency, pointing to our forked frontier project:

# Temporary, leaks the std feature in the non-std env. Until https://github.com/AstarNetwork/frontier/pull/84 is merged.
[patch."https://github.com/AstarNetwork/frontier.git"]
fp-account = { git = "https://github.com/GoldenGateGGX/frontier.git", branch = "polkadot-v0.9.40" }

This patch ensures that we are actually utilizing our own forked version of the frontier project rather than the one under AstarNetwork.

Copy link
Contributor

@uchu uchu left a comment

Choose a reason for hiding this comment

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

Please see why we include code from AstarNetwork instead of forking it and including from our own repo

Cargo.toml Outdated Show resolved Hide resolved
node/src/benchmarking.rs Outdated Show resolved Hide resolved
@luckychacha luckychacha requested review from akorchyn and uchu September 8, 2023 05:36
@akorchyn
Copy link
Contributor

akorchyn commented Nov 2, 2023

Please see why we include code from AstarNetwork instead of forking it and including from our own repo

No much sense to be honest to fork everything we use. We don't plan to adjust it, so why bother forking.

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.

3 participants