Add EvmMapping fragment handling #772
Conversation
|
Need firstly merge #773 |
| #[error( | ||
| "for the provided jormungandr account: {} or evm account: {} mapping is already exist", .0.to_string(), .1.to_string() | ||
| )] | ||
| ExistedMapping(JorAddress, EvmAddress), |
There was a problem hiding this comment.
I am following to the spec, so this behavior has been documented. Of course later we can update it and override existing mapping with the new data, but at least it does not seems that we essentially need this functionality at this moment, so we are following the simplest approach.
| pub last_rewards: LastRewards, | ||
| #[cfg(feature = "evm")] | ||
| pub evm_state: chain_evm::state::AccountState, | ||
| pub extra: Extra, |
There was a problem hiding this comment.
what if AccountState is instead the Extra parameter here?
There was a problem hiding this comment.
It is also an option, but I would prefer to add a new field because it does not leads to the huge update related for replacing in all parts usage of the Account<chain_evm::state::AccountState> with the Account<()> as it is used currently.
And generally I want to suggest to remove this optional extra field, as it is not used and defined as () type.
There was a problem hiding this comment.
What about plans to support other virtual machines / smart contract platforms (eBPF, WASM, ...)? Would it be easier to treat all those states as the Extra instead of adding an hardcoded field for each one?
There was a problem hiding this comment.
Would it be easier to treat all those states as the Extra instead of adding an hardcoded field for each one?
I am not sure that it will be easier, because still we will have a separate struct with which we will define this Extra generic type. But again we will be need to correctly update AccountState typing in all places where it is initialized, that is does not look like as an easy update.
Instead of adding just a new field and modify a few places.
There was a problem hiding this comment.
AccountState typing in all places where it is initialized
I would expect that to be just a type definition in one place and then compiler inference all the way down, but I may be wrong.
To be clear, I don't think adding a new field is a wrong solution, I'm just a bit worried of bloating the struct with many fields, and I'm wondering why do we even have that extra field if we don't use it.
There was a problem hiding this comment.
I am also wondering what is the purpose of the extra field for the AccountState 😊
| address | ||
| ))?; | ||
|
|
||
| let account = self |
There was a problem hiding this comment.
self.account(address) would avoid double access
There was a problem hiding this comment.
It is no appropriate to use self.account(address) at this place because this function returns EvmAccount instead of JorAccount which is needed at this place. And as addition we have different error handing.
There was a problem hiding this comment.
But ultimately you need the EvmAccount in line 121, don't you?
There was a problem hiding this comment.
Yes, you are right, have missed that part. But the most crucial part is to treat an error in this place.
let address = self
.evm
.address_mapping
.jor_address(&address)
.ok_or_else(|| {
ExitError::Other(
format!(
"Can not find corresponding jormungadr account for the evm account: {}",
address
)
.into(),
)
})?
.clone();
And it is something different that we want inside the fn account(&self, evm_address: &EvmAddress)
| Some(account) => { | ||
| self.accounts = self | ||
| .accounts | ||
| .evm_update( |
There was a problem hiding this comment.
evm_update interface suggests it's used to create an account if it does not exist but a new account it never create in practice this way AFAIK
There was a problem hiding this comment.
Yes, you are right, it is not used in this PR, but such option will be used later with the evm contract mapping implementation.
There was a problem hiding this comment.
Maybe it could be useful to add two different methods so that invocations like this one are cleaner?
There was a problem hiding this comment.
Yes, you are right, it is not used in this PR, but such option will be used later with the evm contract mapping implementation.
Sorry, was mistaken and confused you. We probably can create a new account because of
let account = self
.accounts
.get_state(&address)
.cloned()
.unwrap_or_else(|_| JorAccount::new(Value::zero(), ()));
chain-evm/src/machine.rs
Outdated
| source.balance = match source.balance.checked_sub(transfer.value) { | ||
| source.balance = match source | ||
| .balance | ||
| .checked_sub(transfer.value.try_into().map_err(|_| ExitError::OutOfGas)?) |
There was a problem hiding this comment.
Can the failed try_into() be caused by value being bigger than what a balance can hold? In that case I think it's better to have a more descriptive error message, since I'm sure this can happen in a lot of other places as well
There was a problem hiding this comment.
Yes, it is possible, we can throw something similar with ExitError::Other("Balance overflow".into())), but the purpose of throwing ExitError::OutOfGas it is if value which we want to spend is overflowed it is obviously also means that you are trying to spend more than you have ``ExitError::OutOfGaserror in this case. So IMO it is not a huge difference between those two erros types, but I agreeExitError::Other("Balance overflow".into()))` reflects the core problem, so lets throw such error.
There was a problem hiding this comment.
it is if value which we want to spend is overflowed it is obviously also means that you are trying to spend more than you have
Then wouldn't OutOfFund be more appropriate? OutOfGas has a different meaning
2 participants
No description provided.