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

feat: add unused import pass #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions examples/unused_import/erc20_fail.vy
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# @version ^0.3.7

from vyper.interfaces import ERC20
149 changes: 149 additions & 0 deletions examples/unused_import/erc20_implement_pass.vy
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
# @version ^0.3.7

# @dev Implementation of ERC-20 token standard.
# @author Takayuki Jimba (@yudetamago)
# https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20.md

Comment on lines +3 to +6
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep only relevant code in those examples, as you did everywhere else. Starting by all those comments, and I'll suggest other removals.

Suggested change
# @dev Implementation of ERC-20 token standard.
# @author Takayuki Jimba (@yudetamago)
# https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20.md

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is from vyper's examples so I prefer leaving it as it is.

from vyper.interfaces import ERC20
from vyper.interfaces import ERC20Detailed

implements: ERC20
implements: ERC20Detailed

event Transfer:
sender: indexed(address)
receiver: indexed(address)
value: uint256

event Approval:
owner: indexed(address)
spender: indexed(address)
value: uint256

name: public(String[32])
symbol: public(String[32])
decimals: public(uint8)

# NOTE: By declaring `balanceOf` as public, vyper automatically generates a 'balanceOf()' getter
# method to allow access to account balances.
# The _KeyType will become a required parameter for the getter and it will return _ValueType.
# See: https://vyper.readthedocs.io/en/v0.1.0-beta.8/types.html?highlight=getter#mappings
balanceOf: public(HashMap[address, uint256])
# By declaring `allowance` as public, vyper automatically generates the `allowance()` getter
allowance: public(HashMap[address, HashMap[address, uint256]])
# By declaring `totalSupply` as public, we automatically create the `totalSupply()` getter
totalSupply: public(uint256)
minter: address


@external
def __init__(_name: String[32], _symbol: String[32], _decimals: uint8, _supply: uint256):
init_supply: uint256 = _supply * 10 ** convert(_decimals, uint256)
self.name = _name
self.symbol = _symbol
self.decimals = _decimals
self.balanceOf[msg.sender] = init_supply
self.totalSupply = init_supply
self.minter = msg.sender
log Transfer(empty(address), msg.sender, init_supply)



@external
def transfer(_to : address, _value : uint256) -> bool:
"""
@dev Transfer token for a specified address
@param _to The address to transfer to.
@param _value The amount to be transferred.
"""
# NOTE: vyper does not allow underflows
# so the following subtraction would revert on insufficient balance
self.balanceOf[msg.sender] -= _value
self.balanceOf[_to] += _value
log Transfer(msg.sender, _to, _value)
return True


@external
def transferFrom(_from : address, _to : address, _value : uint256) -> bool:
"""
@dev Transfer tokens from one address to another.
@param _from address The address which you want to send tokens from
@param _to address The address which you want to transfer to
@param _value uint256 the amount of tokens to be transferred
"""
# NOTE: vyper does not allow underflows
# so the following subtraction would revert on insufficient balance
self.balanceOf[_from] -= _value
self.balanceOf[_to] += _value
# NOTE: vyper does not allow underflows
# so the following subtraction would revert on insufficient allowance
self.allowance[_from][msg.sender] -= _value
log Transfer(_from, _to, _value)
return True


@external
def approve(_spender : address, _value : uint256) -> bool:
"""
@dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender.
Beware that changing an allowance with this method brings the risk that someone may use both the old
and the new allowance by unfortunate transaction ordering. One possible solution to mitigate this
race condition is to first reduce the spender's allowance to 0 and set the desired value afterwards:
https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
@param _spender The address which will spend the funds.
@param _value The amount of tokens to be spent.
"""
self.allowance[msg.sender][_spender] = _value
log Approval(msg.sender, _spender, _value)
return True


@external
def mint(_to: address, _value: uint256):
"""
@dev Mint an amount of the token and assigns it to an account.
This encapsulates the modification of balances such that the
proper events are emitted.
@param _to The account that will receive the created tokens.
@param _value The amount that will be created.
"""
assert msg.sender == self.minter
assert _to != empty(address)
self.totalSupply += _value
self.balanceOf[_to] += _value
log Transfer(empty(address), _to, _value)


@internal
def _burn(_to: address, _value: uint256):
"""
@dev Internal function that burns an amount of the token of a given
account.
@param _to The account whose tokens will be burned.
@param _value The amount that will be burned.
"""
assert _to != empty(address)
self.totalSupply -= _value
self.balanceOf[_to] -= _value
log Transfer(_to, empty(address), _value)


@external
def burn(_value: uint256):
"""
@dev Burn an amount of the token of msg.sender.
@param _value The amount that will be burned.
"""
self._burn(msg.sender, _value)


@external
def burnFrom(_to: address, _value: uint256):
"""
@dev Burn an amount of the token from a given account.
@param _to The account whose tokens will be burned.
@param _value The amount that will be burned.
"""
self.allowance[_to][msg.sender] -= _value
self._burn(_to, _value)
Comment on lines +100 to +149
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@external
def mint(_to: address, _value: uint256):
"""
@dev Mint an amount of the token and assigns it to an account.
This encapsulates the modification of balances such that the
proper events are emitted.
@param _to The account that will receive the created tokens.
@param _value The amount that will be created.
"""
assert msg.sender == self.minter
assert _to != empty(address)
self.totalSupply += _value
self.balanceOf[_to] += _value
log Transfer(empty(address), _to, _value)
@internal
def _burn(_to: address, _value: uint256):
"""
@dev Internal function that burns an amount of the token of a given
account.
@param _to The account whose tokens will be burned.
@param _value The amount that will be burned.
"""
assert _to != empty(address)
self.totalSupply -= _value
self.balanceOf[_to] -= _value
log Transfer(_to, empty(address), _value)
@external
def burn(_value: uint256):
"""
@dev Burn an amount of the token of msg.sender.
@param _value The amount that will be burned.
"""
self._burn(msg.sender, _value)
@external
def burnFrom(_to: address, _value: uint256):
"""
@dev Burn an amount of the token from a given account.
@param _to The account whose tokens will be burned.
@param _value The amount that will be burned.
"""
self.allowance[_to][msg.sender] -= _value
self._burn(_to, _value)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Same comment as above.

9 changes: 9 additions & 0 deletions examples/unused_import/erc20_use_pass.vy
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# @version ^0.3.7

from vyper.interfaces import ERC20


@external
def foo(a: address) -> uint256:
x: uint256 = ERC20(a).balanceOf(a)
return x
8 changes: 6 additions & 2 deletions medusa/analysis/analyse.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
from vyper import ast as vy_ast

from medusa.analysis.base import BaseAnalyser
from medusa.analysis.passes import DeadStoreAnalyser, UnusedParamAnalyser
from medusa.analysis.passes import DeadStoreAnalyser, UnusedImportAnalyser, UnusedParamAnalyser

PASSES = {"Dead Store": DeadStoreAnalyser, "Unused Function Parameter": UnusedParamAnalyser}
PASSES = {
"Dead Store": DeadStoreAnalyser,
"Unused Import": UnusedImportAnalyser,
"Unused Function Parameter": UnusedParamAnalyser,
}


def analyse(ast: vy_ast.Module) -> dict[BaseAnalyser, set[vy_ast.VyperNode]]:
Expand Down
1 change: 1 addition & 0 deletions medusa/analysis/passes/__init__.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
from medusa.analysis.passes.dead_store import DeadStoreAnalyser
from medusa.analysis.passes.unused_import import UnusedImportAnalyser
from medusa.analysis.passes.unused_param import UnusedParamAnalyser
51 changes: 51 additions & 0 deletions medusa/analysis/passes/unused_import.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
from vyper import ast as vy_ast

from medusa.analysis.base import BaseAnalyser


class UnusedImportAnalyser(BaseAnalyser):
"""
Check for unused imports.

Example:
```
from vyper.interfaces import ERC20
```

`ERC20` is never used.

This pass does not work for custom interfaces yet.
"""

_id = "Unused Import"

def analyse(self, ast: vy_ast.Module, analysis: dict[BaseAnalyser, set[vy_ast.VyperNode]]):

import_nodes = ast.get_descendants((vy_ast.Import, vy_ast.ImportFrom))

for n in import_nodes:
if isinstance(n, vy_ast.Import):
interface_name = n.alias
elif isinstance(n, vy_ast.ImportFrom):
interface_name = n.name

# Check for `implements`
is_implemented = (
len(
ast.get_descendants(
vy_ast.AnnAssign,
{"target.id": "implements", "annotation.id": interface_name},
)
)
> 0
)
Comment on lines +32 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

This fixes the "implements: interface" check (+ a more pythonic way of checking for a non empty list):

Suggested change
# Check for `implements`
is_implemented = (
len(
ast.get_descendants(
vy_ast.AnnAssign,
{"target.id": "implements", "annotation.id": interface_name},
)
)
> 0
)
# Check for `implements`
is_implemented = bool(
ast.get_descendants(vy_ast.nodes.ImplementsDecl, {"annotation.id": interface_name})
)


# Check for usage as `Interface(address).function()`
is_used = len(ast.get_descendants(vy_ast.Call, {"func.id": interface_name})) > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
is_used = len(ast.get_descendants(vy_ast.Call, {"func.id": interface_name})) > 0
is_used = bool(ast.get_descendants(vy_ast.Call, {"func.id": interface_name}))


if not is_implemented and not is_used:
temp = analysis.get(self, set())
temp.add(n)
analysis[self] = temp

return analysis
3 changes: 3 additions & 0 deletions tests/expectations.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,7 @@
("unused_param/double_alt", {"Unused Function Parameter": 1}),
("unused_param/namespace_single", {"Unused Function Parameter": 1}),
("unused_param/nested_namespace_single", {"Unused Function Parameter": 1}),
("unused_import/erc20_implement_pass", {}),
("unused_import/erc20_use_pass", {}),
("unused_import/erc20_fail", {"Unused Import": 1}),
]