-
Notifications
You must be signed in to change notification settings - Fork 47
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
Create2 [EIP-1014] #426
Create2 [EIP-1014] #426
Conversation
e471f26
to
8430069
Compare
.dialyzer.ignore-warnings
Outdated
apps/evm/lib/evm/machine_state.ex:53: Function subtract_gas/2 has no local return | ||
apps/evm/lib/evm/operation/environmental_information.ex:114: Function calldataload/2 has no local return | ||
apps/evm/lib/evm/operation/environmental_information.ex:117: The call 'Elixir.EVM.Helpers':decode_signed(binary()) breaks the contract (integer() | 'nil') -> 'Elixir.EVM':val() | 'nil' | ||
apps/evm/lib/evm/vm.ex:98: The pattern 'continue' can never match the type {'halt','invalid_instruction' | 'stack_underflow' | 'undefined_instruction'} | ||
apps/evm/lib/evm/operation/system.ex:24: Function create/2 has no local return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it usually means that the function might raise an error. Note that it could be a function call that create/2
is making within it that would raise the error, but the point remains. I think this is telling us that somewhere in create/2
there's a possibility of an error being raised and so the function wouldn't have a local return.
I agree that we should avoid adding dialyzer warnings if we can, especially when we're dealing with evm
code. @ayrat555 did you try to figure out why this is now showing up when it wasn't before? Maybe the new code path added has the possibility of raising an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I'm trying to figure out what causes this warning
apps/evm/lib/evm/address.ex
Outdated
|
||
code_hash = Keccak.kec(binary_init_code) | ||
|
||
(<<255>> <> binary_address <> binary_salt <> code_hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to pull <<255>>
out into a module variable or a constant?
apps/evm/lib/evm/operation/system.ex
Outdated
def selfdestruct( | ||
[refund_address], | ||
%{exec_env: exec_env, sub_state: sub_state} | ||
) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the formatter do this? Can it be a single line?
.dialyzer.ignore-warnings
Outdated
apps/evm/lib/evm/machine_state.ex:53: Function subtract_gas/2 has no local return | ||
apps/evm/lib/evm/operation/environmental_information.ex:114: Function calldataload/2 has no local return | ||
apps/evm/lib/evm/operation/environmental_information.ex:117: The call 'Elixir.EVM.Helpers':decode_signed(binary()) breaks the contract (integer() | 'nil') -> 'Elixir.EVM':val() | 'nil' | ||
apps/evm/lib/evm/vm.ex:98: The pattern 'continue' can never match the type {'halt','invalid_instruction' | 'stack_underflow' | 'undefined_instruction'} | ||
apps/evm/lib/evm/operation/system.ex:24: Function create/2 has no local return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it usually means that the function might raise an error. Note that it could be a function call that create/2
is making within it that would raise the error, but the point remains. I think this is telling us that somewhere in create/2
there's a possibility of an error being raised and so the function wouldn't have a local return.
I agree that we should avoid adding dialyzer warnings if we can, especially when we're dealing with evm
code. @ayrat555 did you try to figure out why this is now showing up when it wasn't before? Maybe the new code path added has the possibility of raising an error?
@@ -382,6 +382,7 @@ defimpl EVM.Interface.AccountInterface, for: Blockchain.Interface.AccountInterfa | |||
EVM.MachineCode.t(), | |||
integer(), | |||
Block.Header.t(), | |||
EVM.address(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're passing nil
in the doc test above, I think dialyzer will immediately say this is incorrect (even if we're suppressing the warning) because this argument should be EVM.address() | nil
. Should we add the nil
to the typespec or does it make sense to never pass nil
into this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, we can't. because created account address is only passed if a contract is being created with opcode ( from evm). Contract creation transactions pass nil
Resolves #421