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

Improve visibility of subsequently defined contracts #298

Merged

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Mar 9, 2021

What was wrong?

This superseded #294

If two contracts are defined in a module, only the second contract can see the interface of the first.

e.g.

# can't see Bar
contract Foo:
   ...

# can see Foo
contract Bar:
   ...

How was it fixed?

  1. Contract analysis is split into multiple passes:
    1 The first pass collects contracts with their function definition but does not inspect function bodies and contract fields
    2. Once every contract was analyzed, we analyze the function bodies and contract field of each contract in a subsequent pass

  2. Because the module scope now contains all contracts it needs a way to filter out the main contract when it is aggregating external_contracts. To do that, name was added to ContractScope (we also have name for BlockScope so there's a precedence for having the name as a scope identifier) so that we can filter out the "self contract" upon aggregation.

  3. The Uniswap demo was updated to use that feature

This PR doesn't yet address issue #299 though.

@cburgdorf cburgdorf force-pushed the christoph/feat/better-contract-visibility branch from 8227dc6 to 5ef34c4 Compare March 9, 2021 11:58
@cburgdorf cburgdorf changed the title Christoph/feat/better contract visibility Improve visibility of subsequently defined contracts Mar 9, 2021
// Contract fields are evaluated in the next pass together with function bodies
// so that they can use other contract types that may only be defined after the
// current contract.
Ok(())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@g-r-a-n-t since fields aren't in the public API of contracts it omitting them here doesn't interfere with the contract representation in the scope. But by delaying analysis to the next pass we can at least support referring to subsequent contracts in contract fields.

@cburgdorf cburgdorf force-pushed the christoph/feat/better-contract-visibility branch from f360ed3 to bf46956 Compare March 9, 2021 14:39
@cburgdorf
Copy link
Collaborator Author

@g-r-a-n-t Here's my second attempt which removes all the machinery to manage partial representation of contracts. I created #299 to track the issue with not being able to use subsequently defined contract types in function definitions.

@cburgdorf cburgdorf requested a review from g-r-a-n-t March 9, 2021 14:41
@codecov-io
Copy link

codecov-io commented Mar 9, 2021

Codecov Report

Merging #298 (bf46956) into master (0d7b317) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #298      +/-   ##
==========================================
+ Coverage   93.44%   93.45%   +0.01%     
==========================================
  Files          56       56              
  Lines        3997     4005       +8     
==========================================
+ Hits         3735     3743       +8     
  Misses        262      262              
Impacted Files Coverage Δ
analyzer/src/traversal/contracts.rs 100.00% <ø> (ø)
analyzer/src/lib.rs 90.22% <100.00%> (+0.14%) ⬆️
analyzer/src/namespace/scopes.rs 95.28% <100.00%> (+0.02%) ⬆️
analyzer/src/traversal/assignments.rs 96.66% <100.00%> (ø)
analyzer/src/traversal/declarations.rs 96.77% <100.00%> (ø)
analyzer/src/traversal/expressions.rs 93.05% <100.00%> (ø)
analyzer/src/traversal/functions.rs 97.41% <100.00%> (ø)
analyzer/src/traversal/module.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d7b317...bf46956. Read the comment docs.

@cburgdorf cburgdorf merged commit b02dda3 into ethereum:master Mar 10, 2021
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