Skip to content
This repository was archived by the owner on Apr 20, 2026. It is now read-only.

New flattening impl#52

Merged
mattsse merged 24 commits into
mainfrom
klkvr/new-flattener
Jan 29, 2024
Merged

New flattening impl#52
mattsse merged 24 commits into
mainfrom
klkvr/new-flattener

Conversation

@klkvr

@klkvr klkvr commented Jan 26, 2024

Copy link
Copy Markdown
Member

Ref foundry-rs/foundry#4034.

Solution

solang-parser AST was not sufficient to rename stuff, because it doesn't collect IDs of declarations and we can't match specific identifier to the declaration. Native solc AST gives such ability, so I've implemented native solc AST visitor.

The implementation of flattening itself consits of several steps:

  1. Find all sources that are needed for target (imports of any depth).
  2. Expore ASTs for all sources and find all top-level declarations (contracts, events, structs, etc).
  3. Find if any top-level declarations names are same
  4. If any duplicates are found, figure out new names for them (e.g. 2 OracleLike interfaces become OracleLike_0 and OracleLike_1)
  5. Walk AST and find all references to top-level declarations. Replace every reference with declaration name. We should update names even for unchanged declarations to deal with aliased imports.
  6. Collect and remove all import directives.
  7. Collect and remove all pragmas and license identifiers.

This flattener implementation can be a full replacer of the current impl for all cases when source being flattened can be compiled via solc.

@klkvr klkvr requested review from mattsse and removed request for DaniPopes and Evalir January 26, 2024 19:42
@klkvr klkvr requested review from DaniPopes and Evalir January 26, 2024 19:50
@klkvr

klkvr commented Jan 26, 2024

Copy link
Copy Markdown
Member Author

This should also fix issues with aliases (like foundry-rs/foundry#2545, foundry-rs/foundry#3265).

Current impl handles aliases renaming via regex'es and may often be inaccurate.

Comment thread src/flatten.rs Outdated
@klkvr

klkvr commented Jan 27, 2024

Copy link
Copy Markdown
Member Author

I've refactored code and added logic for correct processing of references to types declared in scope of contracts.

It requires such workaround because for some reason solc does not give any information about the parent contract for UserDefinedTypeName objects representing types like ParentContract.EnumName. Such node will only have AST ID and location of EnumName definition included.

That way, if we want to rename contract ParentContract, we need to be able to determine that AST ID of EnumName is related to that contract and replace it with ParentContract_i.EnumName

@mattsse mattsse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this looks great!!!

can add some additional tests for #34 ?

Comment thread src/flatten.rs
@mattsse

mattsse commented Jan 27, 2024

Copy link
Copy Markdown
Member

@sakulstra I believe you had some issues with flattening in the past, do you know any projects we should test this against?

@sakulstra

Copy link
Copy Markdown

@mattsse there were a few issues/things i commented in the past:

  • Flatten removes "pragma experimental ABIEncoderV2;" foundry#2378
    e.g. cast etherscan-source -d etherscan/0x7b2a3cf972c3193f26cdec6217d27379b6417bd0 0x7b2a3cf972c3193f26cdec6217d27379b6417bd0 && forge flatten ./etherscan/0x7b2a3cf972c3193f26cdec6217d27379b6417bd0/AToken/@aave/protocol-v2/contracts/protocol/tokenization/AToken.sol --output ./etherscan/0x7b2a3cf972c3193f26cdec6217d27379b6417bd0/Flattened.sol would remove pragma experimental ABIEncoderV2; and therefore creating non-identical(in this case breaking) code.

  • Sort before flatten foundry#6539 not really an issue but more a feature request i guess?

For #4034 specifically we also had some issues with that, but can't recall right now in which contracts. Will check if i can find on monday.

@klkvr

klkvr commented Jan 28, 2024

Copy link
Copy Markdown
Member Author

Added test and fix for foundry-rs/foundry#2378

If any of files contains any experimental pragma, it's getting added to the target

@klkvr

klkvr commented Jan 28, 2024

Copy link
Copy Markdown
Member Author

Pushed solution for foundry-rs/foundry#6539.

compilers/src/flatten.rs

Lines 586 to 624 in ce8e427

/// We want to make order in which sources are written to resulted flattened file
/// deterministic.
///
/// We can't just sort files alphabetically as it might break compilation, because Solidity
/// does not allow base class definitions to appear after derived contract
/// definitions.
///
/// Instead, we sort files by the number of their dependencies (imports of any depth) in ascending
/// order. If files have the same number of dependencies, we sort them alphabetically.
/// Target file is always placed last.
pub fn collect_ordered_deps(
path: &PathBuf,
paths: &ProjectPathsConfig,
graph: &Graph,
) -> Result<Vec<PathBuf>> {
let mut deps = HashSet::new();
collect_deps(path, paths, graph, &mut deps)?;
// Remove path prior counting dependencies
// It will be added later to the end of resulted Vec
deps.remove(path);
let mut path_to_deps_count = HashMap::new();
for path in &deps {
let mut path_deps = HashSet::new();
collect_deps(path, paths, graph, &mut path_deps)?;
path_to_deps_count.insert(path, path_deps.len());
}
let mut path_to_deps_count = path_to_deps_count.into_iter().collect::<Vec<_>>();
path_to_deps_count.sort_by_key(|(path, count)| (*count, path.to_path_buf()));
let mut ordered_deps =
path_to_deps_count.into_iter().map(|(path, _)| path.to_path_buf()).collect::<Vec<_>>();
ordered_deps.push(path.clone());
Ok(ordered_deps)
}

I've also shared it's logic with current flattening implementation and rewritten it a bit as it not longer needs to be recursive.

@mattsse mattsse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice, last cosmetic nits!

Comment thread src/flatten.rs Outdated
Comment thread src/flatten.rs Outdated
@klkvr

klkvr commented Jan 29, 2024

Copy link
Copy Markdown
Member Author

@mattsse pushed fixes

let's merge a bit later, I will now write integration of it into foundry and may decide to change the API of Flattener a little bit

@mattsse mattsse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sgtm!
pending foundry companion pr

@Evalir Evalir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

amazing. some nits on docs only

Comment thread src/config.rs Outdated
Comment on lines +410 to +444
let ordered_deps = collect_ordered_deps(&flatten_target, self, &graph)?;

let mut sources = Vec::new();

let mut result = String::new();

for path in ordered_deps.iter() {
let node_id = graph.files().get(path).ok_or_else(|| {
SolcError::msg(format!("cannot resolve file at {}", path.display()))
})?;
let node = graph.node(*node_id);
let mut content = node.content().to_owned();

for alias in node.imports().iter().flat_map(|i| i.data().aliases()) {
let (alias, target) = match alias {
SolImportAlias::Contract(alias, target) => (alias.clone(), target.clone()),
_ => continue,
};
let name_regex = utils::create_contract_or_lib_name_regex(&alias);
let target_len = target.len() as isize;
let mut replace_offset = 0;
for cap in name_regex.captures_iter(&content.clone()) {
if cap.name("ignore").is_some() {
continue;
}
if let Some(name_match) =
["n1", "n2", "n3"].iter().find_map(|name| cap.name(name))
{
let name_match_range =
utils::range_by_offset(&name_match.range(), replace_offset);
replace_offset += target_len - (name_match_range.len() as isize);
content.replace_range(name_match_range, &target);
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could we add a bit of docs over what's happening here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

will add, however this code was written before me so I might get something wrong haha

Comment thread src/flatten.rs
utils, Graph, Project, ProjectCompileOutput, ProjectPathsConfig, Result,
};

#[derive(Debug, PartialEq, Eq, Hash, Clone)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we add some docs here?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants