Conversation
rrw-zilliqa
left a comment
rrw-zilliqa
left a comment
There was a problem hiding this comment.
More or less makes sense, but:
- I think we should check this against my interop tests in #3637
- There's one point I picked up which I'd like clarification on.
- I'd like to see (read: we're not going to release without) tests on the semantics of RFC075 to check that the behaviour is indeed as specified there, otherwise we will have to have yet another update to fix them. Potentially this should be in another PR, since this one is already quite long, but up to you ..
| if (acc_store.isAccountEvmContract(recipient)) { | ||
| // Workaround before we have full interop: treat EVM contracts as EOA | ||
| // accounts only if there's receiver_address set to 0x0, otherwise revert | ||
| if (!scillaArgs.extras || |
There was a problem hiding this comment.
I think this has the behaviour that:
- if you call Scilla from EVM with receiver_address = 0
- and some contract down the chain sends a message
- and that message is to an EVM contract
- and that EVM contract has a handler for the message
- then we ignore the handler and do nothing.
Which isn't what RFC075 says .. is there a test for this that this code passes?
There was a problem hiding this comment.
Nope, that should be reverted in this case because receiver_address will be missing or (impossible with current impl) be non-zero.
|
Thanks, I tested all changes against extended BasicInterop suite and all tests pass. One tests with _EvmCall tag has been added to this PR. What other semantics would you like check? |
|
@bzawisto - I'd like the semantics from RFC075 checked explicitly please. It's not enough that one use case works - we need to make sure the behaviour corresponds with some set of semantics that we are reasonably sure won't allow exploits to be crafted, and the semantics we've picked are set out in RFC075 - so let's write some tests to explicitly ensure that those semantics are followed - partly to make sure your code is correct (and this would've caught the bug Lukas reported in Slack), and partly to make sure we don't violate them with changes in the future. |
|
@rrw-zilliqa As agreed, this will come in other PRs. |
bzawisto
dismissed
rrw-zilliqa’s stale review
As agreed yesterday I'm merging and will fix it in other PR.
bzawisto
deleted the
ZIL-5257-Implement-missing-things-for-interop-phase-1.5
branch
* Some progress with interop phase 1 * Pick up correct sender value --------- Co-authored-by: bzawisto <bartosz@zilliqa.com>
3 participants
Description
Backward Compatibility
Review Suggestion
Status
Implementation
Integration Test (Core Team)