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

Multiple Native Tokens #1

Merged
merged 13 commits into from
Nov 1, 2024
Merged

Multiple Native Tokens #1

merged 13 commits into from
Nov 1, 2024

Conversation

IaroslavMazur
Copy link
Member

No description provided.

@IaroslavMazur IaroslavMazur marked this pull request as ready for review September 8, 2024 17:27
@IaroslavMazur IaroslavMazur changed the title feat: MNAs EIP V1 The EIP on Multiple Native Assets Sep 8, 2024
@IaroslavMazur
Copy link
Member Author

IaroslavMazur commented Sep 8, 2024

@PaulRBerg, ready for your review

Note 1: I've used "assets", instead of "tokens", throughout the EIP, because it feels more appropriate in this context (vs SabVM)

Note 2: The only EIP section that is yet to be written is the Test Case one. However, I believe there's no point in starting to work on it until (and unless) the general idea is approved on the Ethereum Magicians forum first.

Looking forward to your feedback!

Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

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

Thanks, @IaroslavMazur. Here's a log of the high-level changes I made:

  • I formatted the Markdown using Prettier.
  • Implemented the changes discussed on Slack, i.e., adding new opcodes as opposed to changing existing ones.
  • I removed the BALANCES opcode.
  • I removed all notes about there being a governance process for listing tokens. It's outside the scope of this EIP.
  • I refactored the EIP to say Multiple Native Tokens instead of Multiple Native Assets because (i) this is what it's said in the SabVM code, and (ii) our design isn't meant to envision NFTs, as opposed to Fuel's design, which does incorporate NFTs
  • I polished the Motivation, the Rationale, the Backwards Compatibility, and the Security Considerations section
  • I added a section about Prior Art (i.e. FuelVM inspiration)

And left many other comments below, as well as TODOs in the code.

sablier-labs/sabvm#19

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119 and RFC 8174.

A global `token_id` -> `token_supply` mapping is introduced to keep track of the existing NAs and their circulating supply. This mapping is also being used to validate the supported NAs (a NA is supported iff its id can be found in the mapping). The supply of a NA increases as a result of the `MINT` opcode execution - and decreases during the execution of its `BURN` counterparty. The `token_id` of a NA is the Ethereum address of its associated smart contract.

Copy link
Member

Choose a reason for hiding this comment

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

Wasn't the token_id obtained by hashing the contract address with a subID?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it was - in the design of MNTs for our L2.

For Ethereum, however, I think that it'd be better to only allow one-to-one contract-to-NT relations (i.e. a smart contract may only have 1 associated NT). For extra security - and so that every single NT that is being added would have to be explicitly validated/approved (vs approving a smart contract once - and having it create an unlimited number of NTs afterwards).

With this new design, it's no longer needed to obtain the token ids by hashing: given the 1-1 relation, the id/address of the smart contract can simply be the id of the associated NT.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I like this proposal but for a different reason, which is: simplicity and the laser-focus on replacing ERC-20s (which is the overarching goal of this EIP).

So OK great, let's keep it like this.


P.S. there might have been a misunderstanding when I asked you to start drafting this EIP. I'm sorry for that.

This EIP wasn't meant to be implemented on Ethereum Mainnet itself. It was meant to be proposed as a cool idea that could be explored in EVM-based rollups.

I clarified this in my latest review of the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this proposal but for a different reason, which is: simplicity and the laser-focus on replacing ERC-20s (which is the overarching goal of this EIP).

Agree with the beauty of the added simplicity

P.S. there might have been a misunderstanding when I asked you to start drafting this EIP. I'm sorry for that.
This EIP wasn't meant to be implemented on Ethereum Mainnet itself. It was meant to be proposed as a cool idea that could be explored in EVM-based rollups.

No worries, there's nothing to be sorry for! Because we weren't sure whether we want to go the EIP or RIP route, I needed to pick one in order to have a fixed mental framework to draft the proposal based off which - and picked EIP.

If now you've got more reasons to think we should, instead, make this an RIP, then, I'll look deeper/again into what creating an RIP implies - and what must, therefore, be changed about this proposal for it to qualify as one.

Copy link
Member

Choose a reason for hiding this comment

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

Great.

I don't think the RIP process is formal as to require research — it's just like an EIP but called RIP.

I will have a think about EIPs vs RIPs today — but I am slightly more inclined to go with the former because MNTs could be beneficial for Mainnet, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Contrary to the EIPs (that suggest changes to Ethereum), RIPs are, mainly, focused on suggesting changes that would be beneficial/applicable to all/most EVM Rollups. An example of such changes would be various standards that enhance the interoperability of the rollups - or the ease with which a tool created for a rollup can be migrated to another ones.

If we choose to go for an RIP, instead, we'd need to go through the existing RIPs/ecosystem to make sure our MNT proposal fits well as it is - or there is opportunity for a better design of MNTs, when taking the Rollup "wrapper" architecture as a base for the implementation (instead of that of Ethereum).

EIPS/eip-draft_mna.md Outdated Show resolved Hide resolved
EIPS/eip-draft_mna.md Outdated Show resolved Hide resolved
EIPS/eip-draft_mna.md Outdated Show resolved Hide resolved
EIPS/eip-draft_mna.md Outdated Show resolved Hide resolved
EIPS/eip-draft_mna.md Outdated Show resolved Hide resolved
EIPS/eip-draft_mna.md Outdated Show resolved Hide resolved
Copy link
Member Author

@IaroslavMazur IaroslavMazur left a comment

Choose a reason for hiding this comment

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

@PaulRBerg, thank you for reviewing the PR and for your feedback!

I've replied to your comments, implemented the changes I agree with, leaving the rest of the subjects open for debate.

Also, I've left the TODO inside the Abstract section untouched, and am waiting for us to reach a consensus about the rest of the EIP before re-writing the Abstract section.

Notes:

  • I've removed the DELEGATECALL mentions from the NT-related contexts, because this kind of CALL inherits the transferred tokens from its parent context.
  • I've renamed the ...2 opcodes to NT...
  • I've introduced the transferred_tokens_no value in the opcode and tx structure, in order to be able to detect the ill-formed/ill-transported data sequences.

Looking forward to your further feedback!

EIPS/eip-draft_mna.md Outdated Show resolved Hide resolved
EIPS/eip-draft_mna.md Outdated Show resolved Hide resolved
EIPS/eip-draft_mna.md Outdated Show resolved Hide resolved
EIPS/eip-draft_mna.md Outdated Show resolved Hide resolved
EIPS/eip-draft_mna.md Outdated Show resolved Hide resolved
EIPS/eip-draft_mna.md Outdated Show resolved Hide resolved
The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119 and RFC 8174.

A global `token_id` -> `token_supply` mapping is introduced to keep track of the existing NAs and their circulating supply. This mapping is also being used to validate the supported NAs (a NA is supported iff its id can be found in the mapping). The supply of a NA increases as a result of the `MINT` opcode execution - and decreases during the execution of its `BURN` counterparty. The `token_id` of a NA is the Ethereum address of its associated smart contract.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it was - in the design of MNTs for our L2.

For Ethereum, however, I think that it'd be better to only allow one-to-one contract-to-NT relations (i.e. a smart contract may only have 1 associated NT). For extra security - and so that every single NT that is being added would have to be explicitly validated/approved (vs approving a smart contract once - and having it create an unlimited number of NTs afterwards).

With this new design, it's no longer needed to obtain the token ids by hashing: given the 1-1 relation, the id/address of the smart contract can simply be the id of the associated NT.

@PaulRBerg PaulRBerg changed the title The EIP on Multiple Native Assets Multiple Native Tokens Oct 23, 2024
@PaulRBerg
Copy link
Member

PaulRBerg commented Oct 23, 2024

Great feedback @IaroslavMazur, and thank you for completing the remaining TODOs. I agree with all of your proposed changes, except:

I've removed the DELEGATECALL mentions from the NT-related contexts, because this kind of CALL inherits the transferred tokens from its parent context.

Interesting. So we don't need an NTDELEGATECALL then?

I've introduced the transferred_tokens_no value in the opcode and tx structure

I've renamed this to transferred_tokens_length, for increased clarity and consistency with how this value is typically referred to in the EVM.


Could you please update the abstract?

@IaroslavMazur
Copy link
Member Author

So we don't need an NTDELEGATECALL then?

No.

If Alice NTCALLs - tranferring 10 different NTs - a contract X which, in its turn, DELEGATECALLs another contract Y, then, the code from Y that ends up being executed will (rightly) consider it has been called (directly) by Alice who has passed it the aforementioned 10 NTs.

I've renamed this to transferred_tokens_length, for increased clarity and consistency with how this value is typically referred to in the EVM.

Works for me!

Could you please update the abstract?

Absolutely!

I'll do this last thing, though, once we've resolved all of the active discussions, in order to minimize the eventual back-and-forth changes of the Abstract summary.

@PaulRBerg
Copy link
Member

Oooh, you're right. We don't need NTDELEGATECALL since DELEGATECALL itself doesn't contain a value argument.

Copy link
Member Author

@IaroslavMazur IaroslavMazur left a comment

Choose a reason for hiding this comment

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

Looking great! 🤝

@PaulRBerg PaulRBerg merged commit f7b91d5 into master Nov 1, 2024
@PaulRBerg PaulRBerg deleted the iaro/mnas branch November 1, 2024 14:46
PaulRBerg added a commit that referenced this pull request Nov 1, 2024
---------

Co-authored-by: Paul Razvan Berg <[email protected]>
PaulRBerg added a commit that referenced this pull request Nov 1, 2024
---------

Co-authored-by: Paul Razvan Berg <[email protected]>
PaulRBerg added a commit that referenced this pull request Nov 1, 2024
---------

Co-authored-by: Paul Razvan Berg <[email protected]>
PaulRBerg added a commit that referenced this pull request Nov 7, 2024
---------

Co-authored-by: Paul Razvan Berg <[email protected]>
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.

2 participants