Cross-VM payable calls & XVM tests #988
Conversation
|
/bench shibuya-dev pallet_xvm |
|
Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/5738332977. |
|
Benchmarks have been finished. |
pallets/xvm/src/lib.rs
Outdated
| let source_balance = T::Currency::free_balance(&source); | ||
| let existential_deposit = T::Currency::minimum_balance(); | ||
| let killing_source = source_balance.saturating_sub(value) < existential_deposit; | ||
| let adjusted_value = if killing_source { | ||
| value.saturating_sub(existential_deposit) | ||
| } else { | ||
| value | ||
| }; |
There was a problem hiding this comment.
Why not just let it fail/revert?
Shouldn't pallet-contract (and pallet-balances) already ensure this?
There was a problem hiding this comment.
I think we should fail the call if this happens, otherwise for users it will be an unexpected result.
If I understand correctly, Let's say ED=10, EVM Contract balance is 15 and it tries to send 10 to Wasm contract (nft mint), but 0 (10 - 10) will be sent? That does not seems appropriate
Also I don't quite understand this "Only the first call to a payable contract results less value by ED amount, the following ones will not be affected.", what does that mean?
There was a problem hiding this comment.
This is addressed in the integrate test.
Astar/tests/integration/src/xvm.rs
Line 224 in 22dc6e2
If we let it fail/revert, payable calls from EVM to WASM always fail, until the EVM contract gets some balance above ED before the call. The problem is that EVM contracts start with zero balance, but for payable calls, pallet-contracts will assume transferring the full amount value will kill this EVM contract, and it fails with TransferFailed error. Tho pallet-contracts is wrong, an EVM contract can stay at zero balance.
There was a problem hiding this comment.
For Ash's comment on the unexpected result, IMO ED is typically set reasonably low that it doesn't matter. But we do need a solution to fill the gap between EVM and WASM.
In your example, if you mean the EVM contract has 15 after its payable function called, 5 will be sent instead of 0, otherwise if it's before the payable function called, the full amount is transferred.
Only the first call... means that after a payable call from EVM to WASM is successfully made, this contract will have enough balance for ED because of this adjustment. So for following calls, the full amount will always be transferred.
There was a problem hiding this comment.
I understand now, thanks for explaining :)
In nutshell, we just debit ED from value if evm contract does not have it, that's one time cost after that evm contract has ED so pallet-contracts will not complain.
That means first transfer should be greater than ED, as you said ED is quite low so not a issue, but we should document it somewhere for ourselves and builders
There was a problem hiding this comment.
I understand your concern. From a pure engineering point of view, I agree we should just let it fail as it is. This solution is even simpler to implement. Tho practically, I'm more concerned with the extra friction for adoption.
From my experience, it's quite common for one payable EVM function to call another payable, for instance in DEX. EVM doesn't have the concept of ED, and we'll need to educate Solidity builders about ED and substrate pallet-contracts. Then they have to pay attention to any XVM payable usage, or it just won't work. Like our (rather complicated) underlying account system, it will be another burden for them to understand. IMO this kind of friction should be avoided by design.
To be fair, some edge cases you mentioned would be a concern, but compared to the case "dev forgot to load up another contract using XVM payable after new factory deploy or upgrade, then some features won't work", I don't know which is worse. And it's like Unlimited parameter in XCM: do we prefer to fail all the tx if the weight limit is not right anymore, until we do a runtime upgrade to fix, or by using Unlimited the tx is always successful but users might receive less value?
About the reason why other VMs don't have similar stuff like value changes, I believe it's more because their VM doesn't have to bridge two systems that have different designs. They don't have to deal with account unification either😂. (Very lucky! cc @ashutoshvarma ).
There was a problem hiding this comment.
I can't say which solution is better, as apparently, I'm biased as a human. The fairest way to decide is probably trying it in real life.
To push this PR and XVM work forward, a future-proof way might be: remove the value adjustment and let devs feel the burden. If no Solidity builder is using XVM payable calls, then nobody feels the burden, which is very good. If they find the burden is okay, then nobody feels the pain, good. Or if they find it's quite a friction for adoption and we find it's hard to let them understand the underlying gap, it's never too late to update XVM with value adjustment.
There was a problem hiding this comment.
I can't say which solution is better, as apparently, I'm biased as a human. The fairest way to decide is probably trying it in real life.
Haha, of course! 😂
I don't know what's best either, just thought I'd share my opinion & concerns.
To be fair, some edge cases you mentioned would be a concern, but compared to the case "dev forgot to load up another contract using XVM payable after new factory deploy or upgrade, then some features won't work", I don't know which is worse.
Of course, this is one possible error. I'd argue though that it's easily fixable. Just transfer some balance to the contract.
On the other hand, the cases I described, also suffer from the same problem. Devs will still need to make sure that the contract has ED and if it doesn't, "request" additional funds from the user when calling the payable function to ensure required value will be received. Alternative is that their UI logic accounts for this (which seems more complex). Still requires education.
That being said, I don't consider my comments to be hard requirements nor blockers.
I'm ok if we proceed with it like it is now. Perhaps just add a comment in the code that we can consider an alternative approach in the future.
There was a problem hiding this comment.
Thanks for the input. I'll update to remove the value adjustment for now. And let's mark this as open to discussion with builders. We are building this for them to use anyway. Let's see what the feedback is after they learn about it.
If they don't like it, let's then discuss the solution. There could be other possible options, for instance, we can load up pallet-xvm's pallet account with funds from Astar, and transfer ED amount to EVM contracts automatically if needed.
There was a problem hiding this comment.
I'd same concerns regarding this approach, but thinking back again it's not a big deal since it will happen only for the very first payable call via tha EVM contract, and most of the time it will be the developers of contract themselves calling it for the first time (testing, staging env).
So for users it will not be a very big burden to say (except for the very first one 😆)
EDIT: maybe we can add it as a requirement for EVM XVM payable call that contract should've some balance, so that developers can topup some amount (>ED) during contract deploy.
|
/bench shibuya-dev pallet_xvm |
|
Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/5761257991. |
|
Benchmarks have been finished. |
Minimum allowed line rate is |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
* Add value param to 'XvmCall'. * Fix precompile tests. * Update types in CE & precompiles. * Add EVM call tests for XVM. * New type idiom for EthereumTxInput. * Add XVM integration tests. * More XVM integration tests. * Fix runtime tests. * Move contract deploy helpers to setup.rs. * Update value adjustment for wasm payable calls. * More pallet-xvm unit tests. * Update pallet-xvm weight info. * Apply review suggestions. * Fix runtime tests. * Calling WASM payable from EVM fails if caller contract balance below ED. * Update weight info. * Fix unit tests.
Pull Request Summary
pallet-xvmunit testsNotable changes along with payable calls support:
sourceis changed from the contract caller to the contract address, for security reasons. Otherwise, a malicious contract would be able to do anything on behalf of the caller via XVM. This change also makes the behavior consistent with EVM.Some test cases can't be added in this PR, for instance, the payable calls failing cases can't be added until cross-VM revert is implemented. Will follow-up once the required features are finished.
Check list