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: move value transactions to the correct account #3073

Merged
merged 12 commits into from
Nov 15, 2023
Merged

Conversation

lmoe
Copy link
Contributor

@lmoe lmoe commented Nov 13, 2023

Description of change

Currently all value transactions calling ISCMagic deposit into the 0x1074 account instead of the common account.

This PR does a few things:

  • It will block and revert all value transactions calling contract functions that are not marked as payable.
  • It will extend ISCMagic.Send to enable native value transactions.
    • (This adds Metamask token display support)
  • ISCMagic.Send still supports the other way of sending base tokens (Assets metadata).

Links to any relevant issues

fixes issue #3071
fixes issue #3072

Type of change

Choose a type of change, and delete any options that are not relevant.

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)

How the change has been tested

Two tests were added to prove correct behavior:
TestSendPayableValueTX
TestSendUnpayableValueTX

@lmoe lmoe marked this pull request as ready for review November 13, 2023 16:06
packages/vm/core/evm/evmimpl/iscmagic_handler.go Outdated Show resolved Hide resolved
packages/vm/core/evm/evmimpl/iscmagic_sandbox.go Outdated Show resolved Hide resolved
packages/vm/core/evm/evmtest/evm_test.go Outdated Show resolved Hide resolved
@lmoe lmoe merged commit f86e94b into develop Nov 15, 2023
@lmoe lmoe deleted the fix_evm_send branch November 15, 2023 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants