This is a tracking issue that I'm using to dump my thoughts on things that can be cleaned up in this codebase.
General
- [x] convert to circle-ci
- [x] make codebase pep8 compliant
- [x] add mypy
- [x] use tox
- [ ] use
enum
instead of strings for typ
values.
- [ ] use docstrings instead of comments for class definitions in
vyper.types.types
- [x] Use
functools.wraps
for decorators
- [ ] Use proper semantically meaningful exception classes instead of the base
Exception
- [ ] Create
Opcodes
Enum for easier referencing opcodes.
- [ ] Use of floating point values seems like a foot gun. Maybe use
decimal.Decimal
?
- [ ] Use f-strings
- [ ] Use of magic string in
location == "memory"
should be an Enum.
- [ ] Rename
sha3
to keccak
across codebase.
Specifics
- [ ]
vyper.type.types.NodeType.__eq__
change NodeType.__eq__
to require subclasses to implement and remove eq
hook.
- [ ]
vyper.types.types.parse_type
Takes multiple arguments like sigs
, custom_units
, custom_structs
and
constants
. Current read of the code suggests there are data structures which
have been defined in the code and thus, this gives the parser the ability to
properly parse the AST into these types. If so, I believe this should be
formalized into something akin to the WASM Store
to wrap these data
structures up into a single wrapper for easier and less error prone
portability.
- [ ]
vyper.types.types.StructType
Takes a mapping of field_name -> field_type
for members
argument. This
relies on dictionary ordering (which is admittedly now reliable) but also is a
more difficuult data structure to reliably type hint. Consider using an
iterable of 2-tuples: (('field_a', TypeA), ('field_b', TypeB))
. Benefits are
easier type safety through type hinting. Cons are additional validation step
to ensure field uniqueness.
- [ ]
vyper.types.types.get_size_of_type
Relies on prior knowledge about types. This should probably be an API of the
type classes.
- [ ]
vyper.types.types.has_dynamic_data
Relies on prior knowledge about types to determine. This should probably just
be an API of the type classes.
- [ ]
vyper.functions.signature.signature
Uses non-canonical argz
and kwargz
with z
suffixes. Convert to normal *args, **kwargs
Inner decorator
function isn't using functools.wraps
.
More usage of magic strings where an Enum
is probably better.
Replace use of list
with NamedTuple
.
- [ ]
vyper.compiler.gas_estimate
Uses runtime assert
statement which is easily disabled. Change to explicit exception raising.
- [x]
vyper.compiler.mk_full_signature
Loop uses enumerate to generate indices idx
used to modify abi
at that
indices. Instead, use zip
to pair functions with their abi definitions.
- [ ]
vyper.compiler.get_source_map
Builds dict using mutation. Vendor a version of to_dict
decorator to remove mutation pattern.
- [x]
vyper.compiler.compile_codes
Uses mutable data structure as default for argument
Use of magic strings for output_type
should be constrained to enums.
- [x]
vyper.compiler.compile_code
Uses mutable data structure as default for argument
- [ ]
vyper.compiler.get_asm
constant string mutation (which actual ends up as full object copies) adds
overhead. Converting to a wrapped generator pattern that joins all of the
produced strings at the end should be more performant and remove mutation.
Use of next_symbol
global counter appears to be used to create a unique
marker in the code such as _sym_0
, _sym_1
. Replace with
itertools.count()
. Replace with uuid.uuid4()
. Replace with generator
function that wraps an itertools.count()
style counter. Embed within an
object with a defined lifecycle that gets passed around during compilation.
- [ ]
vyper.compile_lll.is_symbol
Replace use of magic string with something more concrete like a custom class.
Potentially the custom class could handle the mechanism for generating unique
symbols.
- [ ]
vyper.compile_lll.instruction
instruction
is a class and thus should be named using common convention
Instruction
.
- [x]
vyper.compile_lll.apply_line_numbers
Needs to use functools.wraps
for inner function.
- [ ]
vyper.compile_lll.compile_to_assembly
Convert runtime assert
statements to raise proper exceptions.
- [ ]
vyper.compile_lll.compile_to_assembly
Detection of whether something is or is not an opcode is done via
isinstance(value, str) and value in opcodes
. If we had an Opcodes
Enum
this could be as simple as checking if Opcodes(value)
raised an exception or
not.
- [ ]
vyper.compile_lll.compile_to_assembly
Use of inlined opcode string values like "PUSH"
would be better as Enum or constant values.
- [ ]
vyper.compile_lll.compile_to_assembly
Individual blocks in the if/elif/.../else
block could be partitioned off into smaller standalone functions.
- [ ]
vyper.compile_lll.compile_to_assembly
The section that handles clamping values has an if/elif
which is missing a
final else
clause allowing for undefined behavior if none of the conditions
evaluates truthy
- [ ]
vyper.compile_lll.assembly_to_evm
Use formal data structure for line_number_map
instead of dictionary.
- [ ]
vyper.compile_lll.assembly_to_evm
Inside of the if isinstance(item, list)
, the local value line_number_map
is
overwritten which appears to be a potentially destructive mistake as it will
destroy any state that was previously built up locally.
- [ ]
vyper.optimizer.get_int_at
Uses inline logic for converting a value to a signed integer. Should be extracted into utility that is tested.
- [ ]
vyper.util.is_instances
Confusingly named. Very visually similar to isinstance
. Maybe rename to a all_is_instances
- [ ]
vyper.parser.constants.Constants.unroll_constant
Use of fail
flag is antipattern. Restructure to allow inlined exception raising.
- [x]
vyper.parser.context.Context.start_blockscope
and others
The various function pairs like start_blockscope/end_blockscope
,
set_in_assertion
, set_in_for_loop/remove_in_for_loop
should be converted to
use context manager pattern so that exiting the scope cannot be accidentally
forgotten.
- [ ]
vyper.parser.global_ctx.GlobalContext.get_item_name_and_attributes
Returns None
for item name in final block which suggests there is either a
case where something does not have a name or that there is return value
checking occuring somewhere.
- [ ]
vyper.parser.stmt.Statements._check_valid_assign
Does not have a final else
block allowing function to exit if none of the conditions evaluate truthy.
- [ ]
vyper.parser.stmt.Statements.parse_for
Uses implicit else
for final clause type. Should be converted to explicitely
check that the statement is formed as expected and add a final else
block
that raises an invariant exception.