-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Synchronize balances between Linera and the EVM. #4274
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
base: main
Are you sure you want to change the base?
Conversation
2c3de1a
to
aced5dc
Compare
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 be possible to extract the general change in system.rs
to its own PR, separate from the EVM-specific changes?
Yes and this is already done in the PR #4268 |
a3a6559
to
33e5f62
Compare
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.
Some more nit-picks; but maybe someone with a better understanding of EVM can give the approval?
|
||
pub(crate) fn get_revm_instantiation_bytes(value: Vec<u8>) -> Vec<u8> { | ||
use alloy_primitives::Bytes; | ||
use alloy_sol_types::{sol, SolCall}; |
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.
Let's move these to the top and also remove them from the functions below.
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.
It is not that easy. But I arranged the code in a reasonable way I think.
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'm curious, why is it not easy? I mean, I believe you 😅 But it looks like it's just about moving imports, so it shouldn't cause issues?
linera-execution/src/evm/revm.rs
Outdated
// | ||
// So, the correct way is instead to test the existence of the application | ||
// given the application_id. So, we need following function in BaseRuntime: | ||
// fn is_existing_application(&mut self, application_id: ApplicationId) -> Result<bool, ExecutionError>; |
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.
Ideally we'd also handle the case where the application has not yet been used on this chain, but will be. So we'd transfer the balance to the account as if it were a user account, but then, if the application is later used, it should have the correct balance. Or is that already the case?
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.
Yes, I can handle that case in a different way. Will see later.
|
||
fn generic_application_id_to_internal_generic_application_id( | ||
generic_application_id: GenericApplicationId, | ||
) -> InternalGenericApplicationId { |
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.
Should these be From
implementations?
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 don't think so.
We might have a larger usage of this kind of conversion to sol types. But right now, the LineraType.StreamUpdate[]
is a single example of such code.
So, we may reconsider this issue later if this kind of code grows.
ca1da82
to
a764c30
Compare
742c08c
to
3b594da
Compare
linera-base/src/vm.rs
Outdated
} | ||
|
||
/// Creates an internal mutation from value and argument data | ||
pub fn get_evm_mutation( |
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.
Can we just add a constructor new
and write this?
bcs::to_bytes(&EvmOperation::new(value, argument))
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.
Is this possible?
impl EvmOperation {
fn new(value: Amount, argument: <something for abi_encode()>) -> ... { ... }
}
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.
It is not easy to make this pass with the Wasm code.
d90c1a4
to
2b4c1db
Compare
linera-base/src/vm.rs
Outdated
} | ||
|
||
/// Creates an operation from value and argument data. | ||
pub fn get_evm_operation( |
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.
We can do better than this.
use crate::data_types::Amount;
impl EvmOperation {
fn new(amount: Amount, argument: Vec<u8>) -> Self {
Self { ... }
}
fn to_bytes(&self) -> Result<Vec<u8>, bcs::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.
Longer explanation:
- Even if one suspects that
get_evm_operation
might not be the same asEvmOperation::new
, it still sounds like it should return a value of typeEvmOperation
. - Top-level
pub fn
definitions should be rare.
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.
Ok, done.
But top-level fn
functions ought to be ok for test code?
/// The instantiation argument to EVM smart contracts. | ||
/// `value` is the amount being transferred. | ||
#[derive(Default, Serialize, Deserialize)] | ||
pub struct EvmInstantiation { | ||
/// The initial value put in the instantiation of the contract. | ||
pub value: alloy_primitives::U256, | ||
/// The input to the `fn instantiate` of the EVM smart contract. | ||
pub argument: Vec<u8>, | ||
} |
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.
Is it important to have this as a separate type? Could instantiation simply be considered a specific kind of operation?
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.
Yes, we can.
However, Linera does allow the separation between the Operation and Instantiation types. We take advantage of it.
So, of course, we could identify the type, but we would gain nothing from doing that. Only losing clarity.
{ | ||
type Error = ExecutionError; | ||
|
||
/// The `basic_ref` is the function for reading the state of |
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.
The comment seems to be incomplete 😅
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.
Corrected.
let mut batch = Batch::new(); | ||
for (address, account) in &self.changes { | ||
if address == &FAUCET_ADDRESS { | ||
// We do not write the faucet address nor expect any coherency from it. |
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.
Some explanation why could be useful (unless it is already explained elsewhere?).
pub contract_address: Address, | ||
/// The caller to the smart contract | ||
pub caller: Address, | ||
/// The value of the smart contract |
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.
It's unclear to me what this means. Is this the amount bundled with the call, maybe? Or with the instantiation? Could be helpful to elaborate a bit.
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.
To answer: Yes, the amount is bundled with the call. It is also bundled with the instantiation.
However, in the case in question, that is DatabaseRuntime
, it is the contract state.
Therefore, here call and instantiation are not relevant.
if !self.changes.is_empty() { | ||
let account = self.changes.get(&address).unwrap(); | ||
return Ok(Some(account.info.clone())); | ||
// This is the case of service calls with empty storage. |
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.
Is it possible that self.changes
is not empty (so there are uncommitted changes), but storage is not empty as well? Shouldn't we try to read from the storage as well, if self.changes.get
returns None
?
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.
The self.changes
are needed for the service calls.
There are two scenarios for the service calls:
- The contract already exists. Then we access the storage and run the service query.
- The contract does not exist. We need to first instantiate the contract before being able to call it (for the service). However, since the service is not allowed to write to storage, we put the result of the storage initialization to
self.changes
.
|
||
pub(crate) fn get_revm_instantiation_bytes(value: Vec<u8>) -> Vec<u8> { | ||
use alloy_primitives::Bytes; | ||
use alloy_sol_types::{sol, SolCall}; |
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'm curious, why is it not easy? I mean, I believe you 😅 But it looks like it's just about moving imports, so it shouldn't cause issues?
ensure!( | ||
inputs.call_value == U256::ZERO, | ||
EvmExecutionError::NoTransferInRuntimeCall | ||
); |
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'm slightly confused here - this seems to ensure that the value is zero, but the error variant seems to indicate that we want the value to not be zero? Shouldn't this be inputs.call_value != U256::ZERO
, then?
ensure!( | ||
get_value(&inputs.value)? == U256::ZERO, | ||
EvmExecutionError::NoTransferInServices | ||
); |
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.
Same as above.
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.
Service calls carry no transfer by construction.
let application_description = ApplicationDescription { | ||
module_id, | ||
creator_chain_id: chain_id, | ||
block_height, | ||
application_index, | ||
parameters: parameters.clone(), | ||
required_application_ids: Vec::new(), | ||
}; |
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.
Maybe we should just have a method like next_application_id
on the runtime? (And, if possible, use it within create_application
, too - so that the logic is not duplicated.)
ceb0eff
to
f2e0c9f
Compare
f2e0c9f
to
87b7f6f
Compare
/// Converting amount from `U256` to Amount can fail since | ||
/// `Amount` is a `u128`. |
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.
/// Converting amount from `U256` to Amount can fail since | |
/// `Amount` is a `u128`. | |
/// Error converting from `U256` to `Amount`. | |
/// This can fail since `Amount` is a `u128`. |
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.
(Or we just use ArithmeticError::Overflow
?)
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.
Thank you.
I prefer to have a specific error for the conversion.
Motivation
Linera has its native-token (do we call it lineras?), and we need to have synchronicity between it and the balance
of the EVM. This is harder to achieve
Fixes #3756
Proposal
In the EVM world, there is complete identity between transferring ethers and calling a function. This is not so in Linera.
Therefore, some changes have to be made for it to work:
transact_commit
takes an amount being transmitted and the function call. Just like in the EVM world.get_mutation
function that takes an amount, an operation, and encapsulates it into a singleVec<u8>
.fn transfer
function works in the sense that the beneficiary receives the ethers on the next block. Since the beneficiaries of transfers are on the same chain as the source, we can make it sync.Other changes being made:
EvmInstantiation
is introduced that contains the native token at instantiation and the command for instantiating.inputs.rs
.Further needed work:
Test Plan
The CI.
The test that demonstrates the functionality is
test_evm_end_to_end_balance_and_transfer
.Release Plan
Links
None.