feat: refactor for zksync abstraction #195
Conversation
# Conflicts: # boa/integrations/jupyter/constants.py
The local variable isn't kept in Colab
This reverts commit 1ff3c18.
charles-cooper
changed the title
boa/environment.py
Outdated
| @@ -331,3 +336,23 @@ def time_travel( | |||
|
|
|||
| self.evm.patch.timestamp += seconds | |||
| self.evm.patch.block_number += blocks | |||
|
|
|||
| def create_deployer( | |||
There was a problem hiding this comment.
i'm not convinced about this function. i think it would be better for this logic to somehow go into VyperDeployer
There was a problem hiding this comment.
Something like this?
boa/contracts/abi/abi_contract.py
Outdated
| @@ -34,7 +33,8 @@ def __init__(self, abi: dict, contract_name: str): | |||
|
|
|||
| @property | |||
| def name(self) -> str: | |||
| return self._abi["name"] | |||
| # note: the `constructor` definition does not have a name | |||
| return self._abi.get("name") or self._abi["type"] | |||
There was a problem hiding this comment.
what happens when we return "constructor" here? does anything rely on the value of name, or is it just for printing?
There was a problem hiding this comment.
probably returning self._abi.get("name") is more robust, caller has to handle the None case explicitly
There was a problem hiding this comment.
Nothing is relying on it afaik, but this is used for error messages.
If we make this optional, we'll need quite some extra code branches to make mypy happy
how do you feel about making the name "" for the constructor?
boa/contracts/abi/abi_contract.py
Outdated
| @@ -274,13 +281,17 @@ def stack_trace(self, computation: ComputationAPI) -> StackTrace: | |||
| """ | |||
| Create a stack trace for a failed contract call. | |||
| """ | |||
| reason = "" | |||
| if computation.is_error and any(computation.error.args): | |||
There was a problem hiding this comment.
what does any(computation.error.args) do?
There was a problem hiding this comment.
In case of a revert from an assert without reason, computation.error.args will be [None]. In that case it's better to leave reason=""
There was a problem hiding this comment.
would it be better to filter those on the next line?
reason = " ".join(str(arg) for arg in computation.error.args if arg is not None)
| @@ -303,6 +305,9 @@ def _check(cond, msg=""): | |||
| assert len(args) == 1, "multiple args!" | |||
| assert len(kwargs) == 0, "can't mix args and kwargs!" | |||
| err = args[0] | |||
| if isinstance(frame, str): | |||
| # frame for unknown contracts is a string | |||
| return _check(err in frame, f"{frame} does not match {args}") | |||
There was a problem hiding this comment.
_check never returns anything
There was a problem hiding this comment.
yeah this always returns None on purpose. I can put the return in another line
boa/interpret.py
Outdated
| with anchor_compiler_settings(ret): | ||
| _ = ret.bytecode, ret.bytecode_runtime # force compilation to happen | ||
| return ret | ||
|
|
||
| cache_key = str((contract_name, source_code, kwargs)) | ||
| cache_key = str((contract_name, source_code, kwargs, deployer)) |
There was a problem hiding this comment.
do we know that deployer has a sane repr? it could be any callable. we should at least have it on a separate line to be a bit clearer
| cache_key = str((contract_name, source_code, kwargs, deployer)) | |
| assert isinstance(deployer, type) | |
| deployer_id = repr(deployer) # arbitrarily choose a unique str for the deployer | |
| cache_key = str((contract_name, source_code, kwargs, deployer_id)) |
There was a problem hiding this comment.
All classes and functions should have a 'sane' unique string, for example
def x(): pass
repr(x)
Out[3]: '<function x at 0x79c03caf9c60>'Unless of course the repr is overridden but it seems safe to assume that the repr would not conflict with some other random type.
I can add the assertions as you requested
There was a problem hiding this comment.
if it's a function, the repr will have a pointer in it. it is more likely to produce a false cache miss than a false cache hit, but it's still possible.
boa/contracts/abi/abi_contract.py
Outdated
| @@ -62,6 +62,8 @@ def pretty_signature(self) -> str: | |||
|
|
|||
| @cached_property | |||
| def method_id(self) -> bytes: | |||
| if self._abi["type"] == "constructor": | |||
| return b"" # constructors don't have method IDs | |||
There was a problem hiding this comment.
i think this should be handled at the caller really (as is done in vyper_contract.py)
boa/contracts/abi/abi_contract.py
Outdated
| @@ -34,7 +33,8 @@ def __init__(self, abi: dict, contract_name: str): | |||
|
|
|||
| @property | |||
| def name(self) -> str: | |||
| return self._abi["name"] | |||
| # note: the `constructor` definition does not have a name | |||
| return self._abi.get("name", "") | |||
There was a problem hiding this comment.
yea i don't think we should use "" as a default here, i think we should return Optional and just handle the None case at the call sites. like we will get stuff like "" as a key in the overloads dictionary, which seems funky.
or if we really want a name, we can use "__init__" (or "constructor" as i think you originally had).
There was a problem hiding this comment.
yeah constructor was the first workaround I thought of, because it's just clear and simple.
handling it at the call sites is fine, but it creates a lot of boiler plate code
What I did
This PR includes a few small changes to make zksync integration possible:
prepare_calldatafor constructorsEnv.create_deployermethod so it may be overriddensign_typed_datain Google Colab (the address was getting lost)sign_typed_datato receive the full message instead of its components (the schema actually requiresprimaryType.Cute Animal Picture