Skip to content

Commit

Permalink
fix(linking): correctly handle duplicated libraries, make library p…
Browse files Browse the repository at this point in the history
…aths unique and fix path issues (#5364)

* fix: use correct nonce to resolve lib address

* fix: adjust test

* chore: rm stray code

* test: add more runner tests for linking

* fix: correct nonce diagnostic

* fix: adjust test

* chore: forge fmt

* chore: forge fmt

* fix: resolve libraries correctly on dupe name

* test: add more tests

* refactor: simplify

* chore: clippy

* chore: properly strip prefix from all paths on scripting

* chore: fmt

---------

Co-authored-by: Oliver Nordbjerg <hi@notbjerg.me>
  • Loading branch information
Evalir and onbjerg committed Jul 13, 2023
1 parent 0b756cd commit 6fbf017
Show file tree
Hide file tree
Showing 10 changed files with 619 additions and 199 deletions.
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 8 additions & 6 deletions cli/src/cmd/forge/script/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use ethers::{
};
use eyre::{Context, ContextCompat};
use foundry_common::compile;
use foundry_utils::PostLinkInput;
use foundry_utils::{PostLinkInput, ResolvedDependency};
use std::{collections::BTreeMap, fs, str::FromStr};
use tracing::{trace, warn};

Expand All @@ -29,6 +29,7 @@ impl ScriptArgs {
/// Compiles the file with auto-detection and compiler params.
pub fn build(&mut self, script_config: &mut ScriptConfig) -> eyre::Result<BuildOutput> {
let (project, output) = self.get_project_and_output(script_config)?;
let output = output.with_stripped_file_prefixes(project.root());

let mut sources: BTreeMap<u32, String> = BTreeMap::new();

Expand Down Expand Up @@ -80,6 +81,8 @@ impl ScriptArgs {

let mut target_fname = dunce::canonicalize(&self.path)
.wrap_err("Couldn't convert contract path to absolute path.")?
.strip_prefix(project.root())
.wrap_err("Couldn't strip project root from contract path.")?
.to_str()
.wrap_err("Bad path to string.")?
.to_string();
Expand Down Expand Up @@ -117,7 +120,6 @@ impl ScriptArgs {
sender,
nonce,
&mut extra_info,
|file, key| (format!("{key}.json:{key}"), file, key),
|post_link_input| {
let PostLinkInput {
contract,
Expand All @@ -127,14 +129,14 @@ impl ScriptArgs {
dependencies,
} = post_link_input;

fn unique_deps(deps: Vec<(String, Bytes)>) -> Vec<(String, Bytes)> {
fn unique_deps(deps: Vec<ResolvedDependency>) -> Vec<(String, Bytes)> {
let mut filtered = Vec::new();
let mut seen = HashSet::new();
for (dep, bytes) in deps {
if !seen.insert(dep.clone()) {
for dep in deps {
if !seen.insert(dep.id.clone()) {
continue
}
filtered.push((dep, bytes));
filtered.push((dep.id, dep.bytecode));
}

filtered
Expand Down
28 changes: 21 additions & 7 deletions forge/src/multi_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,14 @@ use foundry_evm::{
},
revm,
};
use foundry_utils::PostLinkInput;
use foundry_utils::{PostLinkInput, ResolvedDependency};
use rayon::prelude::*;
use revm::primitives::SpecId;
use std::{collections::BTreeMap, path::Path, sync::mpsc::Sender};
use std::{
collections::{BTreeMap, HashSet},
path::Path,
sync::mpsc::Sender,
};

pub type DeployableContracts = BTreeMap<ArtifactId, (Abi, Bytes, Vec<Bytes>)>;

Expand Down Expand Up @@ -242,14 +246,26 @@ impl MultiContractRunnerBuilder {
// create a mapping of name => (abi, deployment code, Vec<library deployment code>)
let mut deployable_contracts = DeployableContracts::default();

fn unique_deps(deps: Vec<ResolvedDependency>) -> Vec<ResolvedDependency> {
let mut filtered = Vec::new();
let mut seen = HashSet::new();
for dep in deps {
if !seen.insert(dep.id.clone()) {
continue
}
filtered.push(dep);
}

filtered
}

foundry_utils::link_with_nonce_or_address(
ArtifactContracts::from_iter(contracts),
&mut known_contracts,
Default::default(),
evm_opts.sender,
U256::one(),
&mut deployable_contracts,
|file, key| (format!("{key}.json:{key}"), file, key),
|post_link_input| {
let PostLinkInput {
contract,
Expand All @@ -258,6 +274,7 @@ impl MultiContractRunnerBuilder {
extra: deployable_contracts,
dependencies,
} = post_link_input;
let dependencies = unique_deps(dependencies);

// get bytes
let bytecode =
Expand All @@ -278,10 +295,7 @@ impl MultiContractRunnerBuilder {
(
abi.clone(),
bytecode,
dependencies
.into_iter()
.map(|(_, bytecode)| bytecode)
.collect::<Vec<_>>(),
dependencies.into_iter().map(|dep| dep.bytecode).collect::<Vec<_>>(),
),
);
}
Expand Down
40 changes: 33 additions & 7 deletions forge/tests/it/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,6 @@ async fn test_core() {
"core/PaymentFailure.t.sol:PaymentFailureTest",
vec![("testCantPay()", false, Some("EvmError: Revert".to_string()), None, None)],
),
(
"core/LibraryLinking.t.sol:LibraryLinkingTest",
vec![
("testDirect()", true, None, None, None),
("testNested()", true, None, None, None),
],
),
("core/Abstract.t.sol:AbstractTest", vec![("testSomething()", true, None, None, None)]),
(
"core/FailingTestAfterFailedSetup.t.sol:FailingTestAfterFailedSetupTest",
Expand All @@ -83,6 +76,39 @@ async fn test_core() {
);
}

#[tokio::test(flavor = "multi_thread")]
async fn test_linking() {
let mut runner = runner().await;
let results = runner.test(&Filter::new(".*", ".*", ".*linking"), None, test_opts()).await;

assert_multiple(
&results,
BTreeMap::from([
(
"linking/simple/Simple.t.sol:SimpleLibraryLinkingTest",
vec![("testCall()", true, None, None, None)],
),
(
"linking/nested/Nested.t.sol:NestedLibraryLinkingTest",
vec![
("testDirect()", true, None, None, None),
("testNested()", true, None, None, None),
],
),
(
"linking/duplicate/Duplicate.t.sol:DuplicateLibraryLinkingTest",
vec![
("testA()", true, None, None, None),
("testB()", true, None, None, None),
("testC()", true, None, None, None),
("testD()", true, None, None, None),
("testE()", true, None, None, None),
],
),
]),
);
}

#[tokio::test(flavor = "multi_thread")]
async fn test_logs() {
let mut runner = runner().await;
Expand Down
17 changes: 8 additions & 9 deletions testdata/cheats/Broadcast.t.sol
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
// SPDX-License-Identifier: Unlicense
pragma solidity 0.8.19;
pragma solidity 0.8.18;

import "ds-test/test.sol";
import "./Vm.sol";

library F {
function t2() public pure returns (uint256) {
return 1;
}
}

contract Test is DSTest {
uint256 public changed = 0;

Expand All @@ -27,12 +33,6 @@ contract Test is DSTest {
}
}

library F {
function t2() public pure returns (uint256) {
return 1;
}
}

contract BroadcastTest is DSTest {
Vm constant vm = Vm(HEVM_ADDRESS);

Expand Down Expand Up @@ -148,10 +148,9 @@ contract BroadcastTest is DSTest {
}

function deployNoArgs() public {
vm.broadcast();
vm.startBroadcast();
Test test1 = new Test();

vm.startBroadcast();
Test test2 = new Test();
vm.stopBroadcast();
}
Expand Down
126 changes: 126 additions & 0 deletions testdata/linking/duplicate/Duplicate.t.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
// SPDX-License-Identifier: Unlicense
pragma solidity 0.8.18;

import "ds-test/test.sol";

// Linking scenario: contract has many dependencies, some of which appear to the linker
// more than once.
//
// Each library should only have its address computed once

library A {
function increment(uint256 number, uint256 number2) external pure returns (uint256) {
return number + number2;
}
}

library B {
function subtract(uint256 number) external pure returns (uint256) {
return number - 1;
}
}

library C {
function double(uint256 number) external pure returns (uint256) {
return A.increment(number, 0) + A.increment(number, 0);
}
}

library D {
function half(uint256 number) external pure returns (uint256) {
return number / 2;
}

function sub2(uint256 number) external pure returns (uint256) {
return B.subtract(number);
}
}

library E {
function pow(uint256 number, uint256 exponent) external pure returns (uint256) {
return number ** exponent;
}

function quadruple(uint256 number) external pure returns (uint256) {
return C.double(number) + C.double(number);
}
}

contract LibraryConsumer {
uint256 public number;

function setNumber(uint256 newNumber) public {
number = newNumber;
}

function increment() public {
number++;
}

function add(uint256 num) external returns (uint256) {
number = num;
return A.increment(num, 1);
}

function sub(uint256 num) external returns (uint256) {
number = num;
return B.subtract(num);
}

function mul(uint256 num) external returns (uint256) {
number = num;
return C.double(num);
}

function div(uint256 num) external returns (uint256) {
number = num;
return D.half(num);
}

function pow(uint256 num, uint256 exponent) external returns (uint256) {
number = num;
return E.pow(num, exponent);
}

function sub2(uint256 num) external returns (uint256) {
number = num;
return D.sub2(num);
}

function quadruple(uint256 num) external returns (uint256) {
number = num;
return E.quadruple(num);
}
}

contract DuplicateLibraryLinkingTest is DSTest {
LibraryConsumer consumer;

function setUp() public {
consumer = new LibraryConsumer();
}

function testA() public {
assertEq(consumer.add(1), 2, "library call failed");
}

function testB() public {
consumer.setNumber(1);
assertEq(consumer.sub(1), 0, "library call failed");
}

function testC() public {
consumer.setNumber(2);
assertEq(consumer.mul(2), 4, "library call failed");
}

function testD() public {
consumer.setNumber(2);
assertEq(consumer.div(2), 1, "library call failed");
}

function testE() public {
consumer.setNumber(2);
assertEq(consumer.quadruple(2), 8, "library call failed");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ pragma solidity 0.8.18;

import "ds-test/test.sol";

// Linking scenario: contract with a library that depends on a library

library Lib {
function plus100(uint256 a) public pure returns (uint256) {
return a + 100;
Expand All @@ -25,7 +27,7 @@ contract LibraryConsumer {
}
}

contract LibraryLinkingTest is DSTest {
contract NestedLibraryLinkingTest is DSTest {
LibraryConsumer consumer;

function setUp() public {
Expand Down
30 changes: 30 additions & 0 deletions testdata/linking/simple/Simple.t.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// SPDX-License-Identifier: Unlicense
pragma solidity 0.8.18;

import "ds-test/test.sol";

// Linking scenario: contract with one library

library Lib {
function plus100(uint256 a) public pure returns (uint256) {
return a + 100;
}
}

contract LibraryConsumer {
function consume(uint256 a) public pure returns (uint256) {
return Lib.plus100(a);
}
}

contract SimpleLibraryLinkingTest is DSTest {
LibraryConsumer consumer;

function setUp() public {
consumer = new LibraryConsumer();
}

function testCall() public {
assertEq(consumer.consume(1), 101, "library call failed");
}
}
Loading

0 comments on commit 6fbf017

Please sign in to comment.