New flattening impl #52

klkvr · 2024-01-26T19:42:24Z

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:

Find all sources that are needed for target (imports of any depth).
Expore ASTs for all sources and find all top-level declarations (contracts, events, structs, etc).
Find if any top-level declarations names are same
If any duplicates are found, figure out new names for them (e.g. 2 OracleLike interfaces become OracleLike_0 and OracleLike_1)
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.
Collect and remove all import directives.
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 · 2024-01-26T19:56:14Z

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.

src/flatten.rs

klkvr · 2024-01-27T04:39:47Z

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

this looks great!!!

can add some additional tests for #34 ?

src/flatten.rs

mattsse · 2024-01-27T15:01:11Z

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

sakulstra · 2024-01-27T22:23:33Z

@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 · 2024-01-28T00:14:00Z

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 · 2024-01-28T02:48:55Z

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

nice, last cosmetic nits!

src/flatten.rs

klkvr · 2024-01-29T11:08:15Z

@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

sgtm!
pending foundry companion pr

Evalir

amazing. some nits on docs only

Evalir · 2024-01-29T14:15:16Z

src/config.rs

+        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);
+                    }
+                }
+            }


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

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

Evalir · 2024-01-29T14:15:33Z

src/flatten.rs

+    utils, Graph, Project, ProjectCompileOutput, ProjectPathsConfig, Result,
+};
+
+#[derive(Debug, PartialEq, Eq, Hash, Clone)]


can we add some docs here?

klkvr added 4 commits January 26, 2024 16:50

Fix AST

4e6fd0e

Visitor

ddb5940

Flattener

590b3cf

tests

4a4bcad

klkvr requested review from DaniPopes and Evalir as code owners January 26, 2024 19:42

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

clippy

02a0eb1

klkvr requested review from Evalir and DaniPopes January 26, 2024 19:50

klkvr added 2 commits January 26, 2024 22:01

clippy 2

8060b1d

chore: renames + comment fix

28fe73f

DaniPopes reviewed Jan 26, 2024

View reviewed changes

src/flatten.rs Outdated Show resolved Hide resolved

klkvr added 2 commits January 26, 2024 22:41

formatting update

7bce316

Refactor + detect references of child items

16d481b

klkvr added 2 commits January 27, 2024 06:51

chore: naming

4424dd3

chore: comments

27879e8

mattsse requested changes Jan 27, 2024

View reviewed changes

src/flatten.rs Show resolved Hide resolved

klkvr added 3 commits January 27, 2024 17:10

Add test for duplicates

bfe3008

fmt

6a439a2

FlatteningResult

4f42c47

Include experimental pragma correctly

8a0cc73

Implement sorting algorithm

ce8e427

klkvr added 2 commits January 28, 2024 04:44

Rewrite current flattening impl

d38dc17

Update tests

1537deb

klkvr added 2 commits January 28, 2024 05:08

small cleanup

8ba890e

fmt

bef1d64

mattsse approved these changes Jan 29, 2024

View reviewed changes

src/flatten.rs Outdated Show resolved Hide resolved

src/flatten.rs Outdated Show resolved Hide resolved

review fixes

3e2e4e7

mattsse approved these changes Jan 29, 2024

View reviewed changes

Merge branch 'main' into klkvr/new-flattener

730cb39

klkvr mentioned this pull request Jan 29, 2024

feat(forge): new flatten implementation foundry-rs/foundry#6936

Merged

Evalir approved these changes Jan 29, 2024

View reviewed changes

klkvr added 2 commits January 29, 2024 17:25

doc

ca6cbb5

Fix old impl

02d2db0

mattsse merged commit 6768886 into main Jan 29, 2024
20 checks passed

DaniPopes deleted the klkvr/new-flattener branch January 29, 2024 16:33

SidestreamColdMelon mentioned this pull request May 31, 2024

Invalid contract flattening with matching interface names #34

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New flattening impl #52

New flattening impl #52

klkvr commented Jan 26, 2024

klkvr commented Jan 26, 2024

klkvr commented Jan 27, 2024

mattsse left a comment

mattsse commented Jan 27, 2024

sakulstra commented Jan 27, 2024

klkvr commented Jan 28, 2024 •

edited

Loading

klkvr commented Jan 28, 2024 •

edited

Loading

mattsse left a comment

klkvr commented Jan 29, 2024

mattsse left a comment

Evalir left a comment

Evalir Jan 29, 2024

klkvr Jan 29, 2024

Evalir Jan 29, 2024

New flattening impl #52

New flattening impl #52

Conversation

klkvr commented Jan 26, 2024

Solution

klkvr commented Jan 26, 2024

klkvr commented Jan 27, 2024

mattsse left a comment

Choose a reason for hiding this comment

mattsse commented Jan 27, 2024

sakulstra commented Jan 27, 2024

klkvr commented Jan 28, 2024 • edited Loading

klkvr commented Jan 28, 2024 • edited Loading

mattsse left a comment

Choose a reason for hiding this comment

klkvr commented Jan 29, 2024

mattsse left a comment

Choose a reason for hiding this comment

Evalir left a comment

Choose a reason for hiding this comment

Evalir Jan 29, 2024

Choose a reason for hiding this comment

klkvr Jan 29, 2024

Choose a reason for hiding this comment

Evalir Jan 29, 2024

Choose a reason for hiding this comment

klkvr commented Jan 28, 2024 •

edited

Loading

klkvr commented Jan 28, 2024 •

edited

Loading