Skip to content
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

fix(levm): create to an existing address #1523

Merged
merged 21 commits into from
Dec 18, 2024
Merged

Conversation

damiramirez
Copy link
Contributor

@damiramirez damiramirez commented Dec 17, 2024

Motivation

The CREATE transaction logic did not handle the case where the address where we the contract will be deploy already exists.

Description

  • Add a check in VM::new() where the transaction is CREATE. If new_contract_address exists in the db and it has nonce or bytecode, we don't create the new account and we don't add it in the cache. We know this is an error and the revert will be handled in transact()
  • Something similar in transact(), we check if the transaction is CREATE and if new_address_acc has code or nonce. In that case we return a TransactionReport with TxResult::Revert.
  • Support the creation of a contract even if the account has balance.

Related #1517

@damiramirez damiramirez added the levm Lambda EVM implementation label Dec 17, 2024
@damiramirez damiramirez self-assigned this Dec 17, 2024
@damiramirez damiramirez changed the title fix(levm): create tx to an existing address fix(levm): create to an existing address Dec 17, 2024
@damiramirez damiramirez marked this pull request as ready for review December 18, 2024 14:22
@damiramirez damiramirez requested a review from a team as a code owner December 18, 2024 14:22
Copy link
Contributor

@lima-limon-inc lima-limon-inc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG(reat)TM

// transaction should be reverted.
if self.is_create() {
let new_address_acc = self.db.get_account_info(initial_call_frame.to);
if new_address_acc.has_code() || new_address_acc.has_nonce() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Should this be an || or a &&?
If an || is needed, then I think a function already exists for it

Suggested change
if new_address_acc.has_code() || new_address_acc.has_nonce() {
if new_address_acc.has_code_or_nonce() {

Having said that, I think the function itself is a bit overkill.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an ||. We created the function because of the specs. It may be a little bit overkill but we can use it I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type of new_address_acc is AccountInfo. This type doesn't have that method :( Maybe we can implement it in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an ||. We created the function because of the specs. It may be a little bit overkill but we can use it I guess.

Noted, thank you for the link Jere!

The type of new_address_acc is AccountInfo. This type doesn't have that method :( Maybe we can implement it in the future.

Oh, my mistake Dami.

crates/vm/levm/src/vm.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@JereSalo JereSalo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good, left some suggestions. The "De Morgan" suggestion doesn't seem to improve code readability, I don't know why I suggested it haha

crates/vm/levm/src/opcode_handlers/system.rs Outdated Show resolved Hide resolved
crates/vm/levm/src/opcode_handlers/system.rs Outdated Show resolved Hide resolved
.checked_add(new_account.balance)
.ok_or(VMError::BalanceOverflow)?;

if !new_account.has_code() && !new_account.has_nonce() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !new_account.has_code() && !new_account.has_nonce() {
if !new_account.has_code_or_nonce() {

Thank you De Morgan (?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary to change it. Maybe it is more readable the original way haha

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes hahaha I think the first implementation is more readable.

damiramirez and others added 4 commits December 18, 2024 16:07
Co-authored-by: Jeremías Salomón <48994069+JereSalo@users.noreply.github.com>
Co-authored-by: Jeremías Salomón <48994069+JereSalo@users.noreply.github.com>
@JereSalo JereSalo enabled auto-merge December 18, 2024 19:38
@JereSalo JereSalo added this pull request to the merge queue Dec 18, 2024
Merged via the queue into main with commit fac0f14 Dec 18, 2024
4 checks passed
@JereSalo JereSalo deleted the fix/create-existing-accounts branch December 18, 2024 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
levm Lambda EVM implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants