-
Notifications
You must be signed in to change notification settings - Fork 6
Support for Factory Contract Dependency in forge create #162
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
base: master
Are you sure you want to change the base?
Conversation
High level comments:
|
Thank you, @soul022, for this pull request. Could you please link this PR to the relevant GitHub issue? You can do this by adding Regarding the parsing of factory dependencies from Solidity code: The idea from the ticket description was that this information would be available directly in the resolc compilation output in |
closes: #130 |
Linked it to issue. |
added comments to upload_child_contract_alloy, removed find_contract_initializations based on new logic |
crates/cli/src/utils/cmd.rs
Outdated
} | ||
|
||
/// Get child contracts from compiled output | ||
pub fn get_child_contracts( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ResolcContract
we have
pub factory_dependencies: Option<BTreeMap<String, String>>,
Is there any reason why you do not use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no ResolcContract in workspace.
Also, Since I am already getting child contracts in pre-existing output object from compiler in
"let output: foundry_compilers::ProjectCompileOutput =
compile::compile_target(&target_path, &project, shell::is_json())?;"
I used it in our usecase
I have just looping through the artifacts from the compiled output to get the child contracts. Attaching below sample artifact array that we get from output, it has both child and parent contract details, get_child_contracts just extracts the child contracts from existing output -
https://gist.github.com/soul022/8f8dc0ef4c0811b05d382d7668f07e40
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will foundry behave when you compile the following code:
// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.7.0 <0.9.0;
library Assert {
function equal(uint256 a, uint256 b) internal pure returns (bool result) {
result = (a == b);
}
}
contract TestAssert {
function checkEquality(uint256 a, uint256 b) public pure returns (string memory) {
Assert.equal(a, b);
return "Values are equal";
}
}
Will foundry upload the Assert library as a child contract?
What about the case where deploy time linking is being used - will your solution still work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is internal library, the compiler simply embeds its code directly into your contract. There’s no separate library contract on-chain and nothing extra to deploy or link at runtime.
My code to get child contracts was treating it as a contract and uploading it, I have modified code in get_child_contracts and added a check to filter the same, so it does not gets uploaded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for deploy time linking i.e external library, it should work as is, As per foundry book -we need to deploy external library first and use linker options (https://paritytech.github.io/foundry-book-polkadot/reference/forge/forge-create.html#linker-options) to create the contract using that external library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried testing linker option for forge create for below contract, but it failed. seems like linker option is not working in foundry-polkadot.
contract - https://gist.github.com/soul022/9ef77e6ac0cf10365b8949fd9bc310fa
logs - https://gist.github.com/soul022/eccb3fc929953e207adafcfd62cba1c9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that the logic you're implementing already exists in the Revive
compiler. I think we should find a way to make factory_dependencies
available.
In the near future, we’ll probably also need to expose unlinked_dependencies
for deploy time linking. WDYT?
@pkhry do you have any opinion on this? You have been looking into the linking already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just add a field to solc
contract artifact in foundry-compilers
. factory-deps
are already present, you just need to forward them. unlinked deps
should probably forwarded the same way as a field on solc::Contract
.
something like this below a la how metadata is currently forwarded:
struct Contract {
...
#[serde(flatten)]
auxiliary: Option<BTreeMap<String, serde_json::Value>>,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also yeah, not a fan of doing something manually if it's already done by the compiler.
crates/forge/src/cmd/create.rs
Outdated
.clone() | ||
.ok_or_eyre("Private key not provided")?; | ||
|
||
let tx_hash = upload_child_contract_alloy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we first check if the contract is already present on-chain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only possible way that I am aware to check if contract is uploaded/exists is to check if storage deposit limit value is 0 using revive api dynamically, there is no possible way to do it right now as it needs substrate node url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the process is already optimized, it won't take up any funds if we try to upload already existing/uploaded contract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pgherveou Do you plan to allow for such a check, or is the current approach good enough in your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should, you would still pay for a transaction that does nothing otherwise
let me look into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry didn't look at this, I think you could check that the code exist using eth_getCode first, that should fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pgherveou , seems like when uploading via magic address, it is not returning contract address, without it we cannot call eth_getCode.
{
"status": "0x1",
"cumulativeGasUsed": "0x0",
"logs": [],
"logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
"type": "0x2",
"transactionHash": "0x2889879e6939bc6a7fde7eb2392329961a9681feb4b60b0ccc3a358f01315bf8",
"transactionIndex": "0x2",
"blockHash": "0xa0f9fd2baca909f5839587037a11203b9831b5401dcb59fdadb61fb406178904",
"blockNumber": "0xa03df",
"gasUsed": "0x57c8d1df958a",
"effectiveGasPrice": "0x4b0",
"from": "0xf24ff3a9cf04c71dbc94d0b566f7a27b94566cac",
"to": "0x6d6f646c70792f70616464720000000000000000",
"contractAddress": null
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad I thought you had an address already, yeah this just upload the code without instantiating it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smiasojed @pgherveou, based on the last discussion, to avoid extra transactions for already uploaded bytecode, we probably need some precompile/check, which would determine if the given bytecode is already present on-chain. For now, @pgherveou suggested to move ahead and this can be picked sooner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly this would be a blocker to merge it, as it incurs extra cost for user
crates/forge/src/cmd/create.rs
Outdated
let output: foundry_compilers::ProjectCompileOutput = | ||
compile::compile_target(&target_path, &project, shell::is_json())?; | ||
|
||
match get_child_contracts(output.clone(), &self.contract.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this introduces new codepath even for solidity contracts, ie should break compat between resolc
and non resolc
versions.
let scaled_encoded_bytes = bytecode.encode(); | ||
let storage_deposit_limit = Compact(10000000000u128); | ||
let encoded_storage_deposit_limit = storage_deposit_limit.encode(); | ||
let combined_hex = "0x3c04".to_string() + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be based on node metadata. Pallet index may be different between runtimes
if let Some(bytecode) = bytecode { | ||
// Skip child contracts (those with 0x3c04 prefix) | ||
let bytecode_slice = bytecode.as_ref(); | ||
if bytecode_slice.len() >= 2 && bytecode_slice[0] == 0x3c && bytecode_slice[1] == 0x04 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this check?
TODO:
|
Motivation
Solution
This change enhances forge create on Substrate-based networks by automatically detecting, encoding, and uploading any factory-declared child contracts before instantiating the main contract:
New dependencies
Adds hex = "0.4.3" and parity-scale-codec (with derive) in crates/forge/Cargo.toml to support SCALE encoding and hex utilities.
find_contract_by_hash helper
Implements a utility in create.rs that scans compiled artifacts, computes each bytecode’s Keccak-256 hash, and returns the matching raw bytes to locate dependencies at runtime.
handle_factory_dependencies flow
Introduces an async routine in create.rs that:
Cleanup & lockfile update
Imports Compact, Encode, and BTreeMap, removes unused code, and bumps Cargo.lock for the new crates.
With this PR, users can simply declare child-contract dependencies in their artifacts and rely on forge create to handle the full upload and instantiation sequence automatically.
PR Checklist
Test Evidance
Contract Tested -
https://gist.github.com/soul022/268ff439b8381bb02839161121066b0a
Logs -
https://gist.github.com/soul022/63b440a61e442ff57081132afdd2da09
closes: #130