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.

Sign up for GitHub

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

Jump to bottom

Support user defined operators #1684

Merged
merged 9 commits into from
Jun 23, 2023
Merged

Support user defined operators #1684

merged 9 commits into from
Jun 23, 2023

Conversation

smonicas
Copy link
Contributor

No description provided.

@montyly montyly mentioned this pull request Feb 25, 2023
Closed
@montyly
Copy link
Member

montyly commented Mar 10, 2023

Can we add tests?

@montyly montyly mentioned this pull request Mar 10, 2023
Closed
0xalpharush
0xalpharush reviewed Mar 13, 2023
View reviewed changes
tests/test_features.py Outdated
for ir in add_function_call.all_slithir_operations():
if isinstance(ir, InternalCall) and ir.function_name == "add":
ok = True
if not ok:
Copy link
Member

Choose a reason for hiding this comment

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

Can just do assert ok

0xalpharush
0xalpharush reviewed Mar 13, 2023
View reviewed changes
tests/test_features.py Outdated
eq_op = contract_t.get_function_from_full_name("eq_op(Int,Int)")
for ir in eq_op.all_slithir_operations():
if isinstance(ir, InternalCall) and ir.function_name == "eq":
return
Copy link
Member

Choose a reason for hiding this comment

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

Can you break and make an assertion instead of returning?

@0xalpharush
Copy link
Member

Instead of reusing ok, can something descriptive of what property is being tested like correctly_does_x be used?

0xalpharush
0xalpharush reviewed Mar 13, 2023
View reviewed changes
slither/solc_parsing/declarations/using_for_top_level.py Outdated
self, operator: str, function_name: str, type_name: TypeAliasTopLevel
) -> None:
for tl_function in self.compilation_unit.functions_top_level:
if tl_function.name == function_name:
Copy link
Member

Choose a reason for hiding this comment

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

This should check that the parameters are the correct type otherwise I think it will run into bugs similar to #1625

@0xalpharush
Copy link
Member

This isn't working on the prb-math's operators https://github.com/PaulRBerg/prb-math/tree/326dda4d85808f234035fbf68089f4dc44f877ba

  File "/Users/alpharush/.pyenv/versions/3.11.1/envs/slither/lib/python3.11/site-packages/slither/core/cfg/node.py", line 679, in slithir_generation
    self._irs = convert_expression(expression, self)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alpharush/.pyenv/versions/3.11.1/envs/slither/lib/python3.11/site-packages/slither/slithir/convert.py", line 140, in convert_expression
    result = apply_ir_heuristics(result, node)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alpharush/.pyenv/versions/3.11.1/envs/slither/lib/python3.11/site-packages/slither/slithir/convert.py", line 1926, in apply_ir_heuristics
    irs = propagate_type_and_convert_call(irs, node)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alpharush/.pyenv/versions/3.11.1/envs/slither/lib/python3.11/site-packages/slither/slithir/convert.py", line 463, in propagate_type_and_convert_call
    new_ins = propagate_types(ins, node)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alpharush/.pyenv/versions/3.11.1/envs/slither/lib/python3.11/site-packages/slither/slithir/convert.py", line 617, in propagate_types
    convert_type_of_high_and_internal_level_call(ir, function_contract)
  File "/Users/alpharush/.pyenv/versions/3.11.1/envs/slither/lib/python3.11/site-packages/slither/slithir/convert.py", line 1625, in convert_type_of_high_and_internal_level_call
    for f in contract.functions
             ^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'functions'

There isn't a contract for internal calls to the UDVT operators. This needs to be thought through carefully as the SolidityImportPlaceholder code was only a temporary solution and not robust #1452.

slither/slither/slithir/convert.py

Lines 1620 to 1640 in 64b2e0c

if ir.function_candidates:
# This path is taken only for SolidityImportPlaceHolder
# Here we have already done a filtering on the potential targets
candidates = ir.function_candidates
else:
candidates = [
f
for f in contract.functions
if f.name == ir.function_name
and f.contract_declarer.name == ir.contract_name
and len(f.parameters) == len(ir.arguments)
]
for import_statement in contract.file_scope.imports:
if import_statement.alias and import_statement.alias == ir.contract_name:
imported_scope = contract.compilation_unit.get_scope(import_statement.filename)
candidates += [
f
for f in list(imported_scope.functions)
if f.name == ir.function_name and len(f.parameters) == len(ir.arguments)
]

@montyly
Copy link
Member

montyly commented Jun 22, 2023

@smonicas : what is the status of the PR?

@montyly montyly merged commit 79579f9 into dev Jun 23, 2023
49 checks passed
@montyly montyly deleted the dev-ud-operators branch June 23, 2023 14:20
@montyly montyly mentioned this pull request Jun 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xalpharush 0xalpharush 0xalpharush left review comments

@montyly montyly Awaiting requested review from montyly montyly is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@smonicas @montyly @0xalpharush