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

feat: native tokens #155

Merged
merged 29 commits into from
Sep 13, 2024
Merged

feat: native tokens #155

merged 29 commits into from
Sep 13, 2024

Conversation

PaulRBerg
Copy link
Member

@PaulRBerg PaulRBerg commented May 28, 2024

Context

A big PR that closes #5, #74 and #145.

This PR is a replacement for #14 and #148, which we have closed as a result of (i) multiple conflicts due to an in-flight merge and (ii) the name change from MNAs to MNTs.

Feedback

I slogged through the rebase, reviewing your work @IaroslavMazur in the process.

Feedback and changelog:

  • Misc improvements, e.g.
    • Keep only drain_balances because this function is needed only for the DAO hardfork
    • Use increase_base_balance_saturating instead of set_base_balance(get_base_balance().saturating_add())
  • Swapped the order of the parameters for BALANCEOF; as mentioned in the spec, the token id comes first
  • Refactored "Native Assets" to "Native Tokens"
  • The opcodes should be defined close to each other in order to (i) minimize rebasing conflicts and (ii) avoid accidental overlaps with planned EIPs
  • Undid the NTCALL, NTDELEGATECALL, and NTCREATE implementations because we're not planning on supporting smart contracts in Genesis (besides MNTs themselves).
  • Undid all SELF-DESTRUCT logic removal; even if we decide to remove this logic, it'd be better to do it in a separate PR.
    • For the future: we don't need an NTCALLCODE opcode, just an NTDELEGATECALL because CALLCODE is a deprecated version of DELEGATECALL.

Rebasing

I will create a separate discussion about our REVM rebasing strategy.

@PaulRBerg PaulRBerg force-pushed the feat/native-tokens branch 2 times, most recently from 7674580 to 1dc47d1 Compare May 28, 2024 14:13

This comment was marked as off-topic.

@PaulRBerg PaulRBerg mentioned this pull request May 28, 2024
2 tasks
@PaulRBerg PaulRBerg changed the base branch from main to forked-revm/main May 28, 2024 14:39
@PaulRBerg PaulRBerg changed the base branch from forked-revm/main to main May 28, 2024 14:39
Co-authored-by: Iaroslav Mazur <[email protected]>
Cargo.lock Outdated Show resolved Hide resolved
crates/interpreter/src/instructions/host.rs Outdated Show resolved Hide resolved
crates/primitives/src/state.rs Outdated Show resolved Hide resolved
crates/revm/src/context/inner_evm_context.rs Outdated Show resolved Hide resolved
@IaroslavMazur
Copy link
Member

Swapped the order of the parameters for BALANCEOF; as mentioned in the spec, the token id comes first

Either way is fine, as long as on the opposite side (i.e. when the opcode arguments are being input), the items are being pushed in the reverse order. Stack is LIFO-based.

Note that in the balance() function, where balance_of() is being called from, the token id is being pushed to the Stack first, and the address - second.

Therefore, either the address must be popped first in balance_of() - or the pushing order in balance() needs to be reversed, as well.

@PaulRBerg
Copy link
Member Author

Either way is fine

Yeah, either way works technically, but this is a matter of consistency and logical flow.

The balances (in the state) are mapped from token IDs to amounts, so it makes sense to have the opcode and the precompile follow the same order.

either the address must be popped first

Yes. Happy to pop the address first, because that's an internal operation that end users need not about.

@IaroslavMazur
Copy link
Member

Yes. Happy to pop the address first, because that's an internal operation that end users need not about.

After having taken a closer look at the other EVM opcodes and how they're implemented in revm, for the sake of consistency, I suggest we follow the existing example of the EXTCODECOPY opcode. In its implementation, the opcode arguments are being popped in their "logical" order.

The above would mean:

  1. popping the token_id, first, and the address - second in balance_of() and
  2. pushing them in reverse order (address, followed by token_id) in balance()

If you agree, I can include this change with the rest of the things I've got to move on top on this PR branch.

@PaulRBerg
Copy link
Member Author

sounds good

@IaroslavMazur IaroslavMazur force-pushed the feat/native-tokens branch 4 times, most recently from 29e3896 to 927a6df Compare May 30, 2024 14:29
feat: EOAs not allowed as Precompile callers

fix: argument push order for BALANCEOF;

test: mntcallvalues via Precompile;
test: fix the caller's Nonce;

refactor: extract the byte-parsing tools to primitives/utilities.
fix: Precompile now returns Transferred Tokens from Call and not Tx

test: get Call Values as a contract

refactor: remove the now-redundant EOA-to-Precompile test
refactor: use a more suggestive naming in the SabVM tests
@IaroslavMazur IaroslavMazur force-pushed the feat/native-tokens branch 4 times, most recently from e99df44 to 4f0cd8a Compare August 26, 2024 18:14
@IaroslavMazur IaroslavMazur force-pushed the feat/native-tokens branch 2 times, most recently from 323a89f to 0ac66cc Compare August 26, 2024 19:17
refactor: Balances -> TokenBalances
test: add some extra verifications in the tests

refactor: make the naming inside the tests more suggestive
refactor: extract the Precompile functionalities into stand-alone
		  functions
docs: document the codebase
@IaroslavMazur
Copy link
Member

@PaulRBerg, the PR is ready to be reviewed

@IaroslavMazur
Copy link
Member

As discussed privately, I'm integrating this PR without @PaulRBerg's review.

@IaroslavMazur IaroslavMazur merged commit 51f5384 into main Sep 13, 2024
22 checks passed
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.

Native Tokens (NTs)
2 participants