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

[Proposal] Custom base token support #15

Closed
thomas-nguy opened this issue Sep 11, 2023 · 9 comments
Closed

[Proposal] Custom base token support #15

thomas-nguy opened this issue Sep 11, 2023 · 9 comments

Comments

@thomas-nguy
Copy link
Member

thomas-nguy commented Sep 11, 2023

Summary:

Right now, only ETH is supported for hyperchain's base token. There are several use cases where a hyperchain would like to have a specific erc20 token as a base token rather than the native ETH.

In this issue, we are proposing to modify the "deposit" and "withdrawal" logic in the zksync contracts so that it could handle custom base token.

Problem definition

To understand the problem, we need to describe how the base token are currently "minted" and "burned" in layer 2.

The balance of the base token for all accounts is maintained by the L2EthToken.sol contract on L2.
The mint and burn can only be called by the bootloader.

Mint

The current "mint" mechanism is controlled by an event emitted by the zksync's diamond contract on L1

event NewPriorityRequest(
  uint256 txId,
  bytes32 txHash,
  uint64 expirationTimestamp,
  L2CanonicalTransaction transaction,
  bytes[] factoryDeps
);

With

 struct L2CanonicalTransaction {
        uint256 txType;
        uint256 from;
        uint256 to;
        uint256 gasLimit;
        uint256 gasPerPubdataByteLimit;
        uint256 maxFeePerGas;
        uint256 maxPriorityFeePerGas;
        uint256 paymaster;
        uint256 nonce;
        uint256 value;
        // In the future, we might want to add some
        // new fields to the struct. The `txData` struct
        // is to be passed to account and any changes to its structure
        // would mean a breaking change to these accounts. To prevent this,
        // we should keep some fields as "reserved".
        // It is also recommended that their length is fixed, since
        // it would allow easier proof integration (in case we will need
        // some special circuit for preprocessing transactions).
        uint256[4] reserved;
        bytes data;
        bytes signature;
        uint256[] factoryDeps;
        bytes paymasterInput;
        // Reserved dynamic type for the future use-case. Using it should be avoided,
        // But it is still here, just in case we want to enable some additional functionality.
        bytes reservedDynamic;
    }

The structure L2CanonicalTransaction contains a reserved field in which the first 32bytes are allocated to the valueToMint (let's call it reserved0 field)

A watcher monitors the Ethereum events emitted from the zksync’s diamond contract and creates a transaction on layer2 for each of this event recorded.

The bootloader, upon processing the transaction, will check the the value specified in the reserved0 field. In case this value is not null, will mint the specified value to the specified destination address.

Some safety checks are implemented to ensure that no value is created from thin air

  • The reserved0 field(value to mint) specified in the event is equal to the transaction msg.value . The zksync contract is used to store all the bridged ETH.

Total ETH mint = Total ETH stored in zksync contract account

  • To reduce the chance of failure, we also ensure that this value is enough to cover the transaction (cost & value) on L2.

Burn

The "burn" mechanism is controlled by the L2EthToken.sol contract on L2. By calling the withdrawal method, the amount specified in the transaction msg.value is burned from the sender account and a message is inserted to the L1_MESSENGER_CONTRACT. The message contains the withdrawal amount and the receiver address.

The eth_sender picks the message, includes it in a L2 transaction, and then encapsulates it in a block.

Upon executing the block on L1, the finalizeEthWithdrawal method from the zksync contract can be called on the block and process any messages it contains.

The ETH will be released to the specified receiver when processing the withdrawal message.

Proposed solution:

We follow the principle on "minimum change"

In this solution, 1 unit of token in L1 (locked) correspond to 1 unit of base token in L2 . The token contract on L1 should follow the ERC20 standard and have 18 decimals.

The only part that requires change in the solution is the "lock" and "unlock" mechanism on the L1 smart contract (mailbox) and the field “valueToMint” be redefined.

The logic of “mint” and “burn” on layer2 remains the same, however it is crucial to implement an Oracle in the future to adjust the gas_price on layer2 according to the related price of the base token against Ethereum (could be a coefficient factor stored in DB and updated by a trusted oracle) as well a mechanism for the operator to convert the base token to ETH on L1 so that it can cover its costs.

We will only describe the changes needed in the smart contract in this issue.

The idea is to introduce a new "lock" and "unlock" mechanism at the Mailbox facet level and that instead of setting the valueToMint based on the msg.value, will use the value corresponding to the amount of ERC20 locked(safeTransfer) to the contract.

  • The msg.value is ignored and instead we will introduce a new input parameter uint256 _baseAmount. User can use this new parameter to specify the amount of token to be bridged.

https://github.com/matter-labs/era-contracts/pull/12/files#diff-df9164d8de562b3612626c482a4d9f1c014d796f1bd4d5dd42a210386a5bcf6dR263

  • In case of mint event, we have to explicitly transfer the ERC20 from the sender address to the contract address (lock)

https://github.com/matter-labs/era-contracts/pull/12/files#diff-df9164d8de562b3612626c482a4d9f1c014d796f1bd4d5dd42a210386a5bcf6dR245

  • In case of withdrawal event, we have to explicitly transfer the ERC20 from the contract address to the sender address

https://github.com/matter-labs/era-contracts/pull/12/files#diff-df9164d8de562b3612626c482a4d9f1c014d796f1bd4d5dd42a210386a5bcf6dR110

  • The logic to decide if we want to use an ERC20 or ETH as base token is defined by the baseTokenAddress value stored by the diamond proxy contract during initialization. This value represents the address of the ERC20 token to be used as base token. If it is zero, then we fallback to the default behavior which is to use ETH as base token

https://github.com/matter-labs/era-contracts/pull/12/files#diff-8c8f228bb83a30dbd6f369160e2016a869e01b68b1c106fc39196742f361d33bR50

  • The logic on L1WethBridge can be completely ignored. WETH can be treated as any ERC20 token

Points to be discussed

  • Should be make the changes in the existing contract or implement in new facets ?

It is possible to introduce a new facet CustomBaseMailBox and implement logic related to custom base token there instead of modifying the existing MailBox facet.

The script would need to be modified to deploy either CustomBaseMailBox or MailBox depending on the deployer use cases.

Pro : Non breaking change
Cons : More facets to maintains. Each changes in one facet would need to be duplicated in the other facet

@thomas-nguy thomas-nguy changed the title [Proposal] Custom base token support [Proposal] Custom base token support (draft) Sep 11, 2023
@thomas-nguy
Copy link
Member Author

Related PR

#12

@thomas-nguy thomas-nguy changed the title [Proposal] Custom base token support (draft) [Proposal] Custom base token support Sep 11, 2023
@githubdoramon
Copy link
Contributor

On initial question @thomas-nguy - Why requiring token to have 18 decimals and don't make this customizable?

@thomas-nguy
Copy link
Member Author

thomas-nguy commented Sep 12, 2023

@githubdoramon
At the EVM layer (L2) the gas token should have an 18 decimals representation (many operations and sdks depends on it for wei/gwei conversion), if we choose a base token that does not have 18 decimals it will create complications.

@githubdoramon
Copy link
Contributor

@githubdoramon At the EVM layer (L2) the gas token should have a 18 decimals representation (many operations and sdks depends on it for wei/gwei conversion), if we choose a base token that does not have 18 decimals it will create complications.

@thomas-nguy
Good point. We could still create the L@ version with 18 decimals regardless of decimals amount on L1, and upon withdrawals leave any remainder on the L2 side when converting back. Not at ALL a priority or needed on the first iteration, but could be a future use case (imagine Circle wants a hyperchain for example).

StanislavBreadless pushed a commit that referenced this issue Sep 14, 2023
update scheduler circuit verification key
@mm-zk
Copy link
Collaborator

mm-zk commented Sep 19, 2023

Hey @thomas-nguy ,

First - thanks for great description of the issue (and a draft PR).

IMHO - I'd be leaning towards doing these changes in separate Facet right now - this will allow faster experimentation. And once the features stabilize, we could add them to the main facet if needed.

@mm-zk
Copy link
Collaborator

mm-zk commented Sep 19, 2023

For the part about:

it is crucial to implement an Oracle in the future to adjust the gas_price on layer2 according to the related price of the base token against Ethereum (could be a coefficient factor stored in DB and updated by a trusted oracle)

I guess we'd have to modify the GasAdjuster (more details here: https://github.com/matter-labs/zksync-era/blob/main/docs/advanced/gas_and_fees.md)

@thomas-nguy
Copy link
Member Author

thomas-nguy commented Sep 20, 2023

Hi @mm-zk .

I agree with the facet approach. I think this is the best way to test new features in the zk contract without breaking existing logic.

The reason I use the "same facet" approach in the PR #12 is to make the changes more obvious to review, but I will re-open a new PR using the new facet approach

Regarding the GasAdjuster, it seems that we have the same idea :)

I have opened up a PR in the zksync repository that give an idea of our implementation

matter-labs/zksync-era#96

Let me know if this approach is acceptable

@jaguard2021
Copy link

Advantages:

Diversification of Base Tokens: Supporting ERC20 as the base token allows for the use of custom tokens, enhancing flexibility and catering to diverse user needs.

"Minimum Change" Principle: The "minimum change" approach can help reduce risks and system complexity by altering only necessary parts without significantly disrupting existing functionalities.

Contract Flexibility:

CustomBaseMailBox: Adding a new facet like CustomBaseMailBox can facilitate the management of logic related to custom base tokens without disturbing the existing Mailbox. This can aid in separating responsibilities and improving code clarity.

Changes to Existing Contract: Implementing changes to the existing contract can provide a non-breaking change solution, but care must be taken to avoid increasing the complexity of the already existing contract.

Disadvantages:

Maintenance Duplication: Opting for an approach with two facets (CustomBaseMailBox and MailBox) may lead to maintenance duplication. Every change in one facet must be manually implemented in the other facet, increasing the risk of errors.

Oracle and Token Conversion: The solution proposes introducing an Oracle in the future to adjust gas_price on Layer 2 and a mechanism for converting the base token to ETH on Layer 1. While this considers costs and stability, the implementation needs careful consideration to avoid introducing additional complexity.

Gas Price Considerations: Adjusting gas_price on Layer 2 based on the price of the base token against Ethereum requires careful consideration and management to avoid potential exploitation or instability in the network.

@shahar4
Copy link
Collaborator

shahar4 commented Nov 13, 2023

@thomas-nguy thanks again for this fantastic issue! I'm closing this for now and will resume the main design.

@shahar4 shahar4 closed this as completed Nov 13, 2023
benceharomi pushed a commit that referenced this issue Nov 17, 2023
jrchatruc referenced this issue in lambdaclass/era-contracts Jan 16, 2024
* A new era

but because it’s a credibly neutral mechanism

* Logo + disclaimer.

* chore(security): add workflow for leaked secrets monitoring

* Update README.md

* Remove Apache license.

* Updating to latest in dev.

* Fair Onboarding Alpha.

* Add comment on EIP-1352

* Updating mirror

* Updating mirror.

* Update README.md

* Updating mirror.

Used 663fede669db3ba66f0941985db304e8bca881e4.

* mirror sync to 7381458849b42

* Mirror to de404a390af2aa37ad23b2a543c5f1b408ca84bf (#11)

* added missing file to mirror  de404a390af2aa37ad (#12)

* fix: bump hh deploy and solc versions (#13)

* Add FOS Templates (#15)

* chore: Syncs common workflows from the template into dev (#16)

* chore: Syncs common workflows from the template into main (#17)

* Syncing dev with main (#26)

Co-authored-by: Marcin M <[email protected]>
Co-authored-by: Dennis <[email protected]>
Co-authored-by: Shahar Kaminsky <[email protected]>
Co-authored-by: Yury Akudovich <[email protected]>

* Boojum integration (#35)

Co-authored-by: Marcin M <[email protected]>
Co-authored-by: Dennis <[email protected]>
Co-authored-by: Shahar Kaminsky <[email protected]>
Co-authored-by: Vlad Bochok <[email protected]>
Co-authored-by: koloz193 <[email protected]>
Co-authored-by: AntonD3 <[email protected]>

* chore: Upgrade to Node v18 (#20)

* feat: Adding compile CI (#21)

* feat: testing CI job (#38)

* ci: testing added

* test: temporarily commenting out failing tests

* ci: cleaned up + added testing

* fix: CI syntax

* ci: added missing "needs" statement

* ci: added missing node-setup

* ci: added missing artifacts for cacheing

* test: xdescribe and xit instead of commenting

* chore: formatting

* Testing framework for bootloader (#14)

* added missing file to mirror  de404a390af2aa37ad (#12)

* POC - works

* test infra creation

* splitting tracers to separate files

* moved hooks to separate file

* larger refactor - nicer error messages

* syncing with newest version

* more bootloader tests and small error fixes

* more tests

* Example with transaction

* small fixes

* small rename

* review and removed dependency on ZKSYNC_HOME

* cargo lock

* updated to public zksync-era

* moved the placeholder so that the generated bootloader code doesn't change

* review

* fix yarn lock

* compiles (currently depending on a local branch)

* remove vscode config

* added bootloader test to CI

* changing CI

* experimenting

* fix

* review feedback

* ci typo

* added bootloader build to cache

* feat: linting CI job (#40)

* feat: linting

* chore: PR template updated

* fix: import order

* lint: solidity compiler-version 0.8.0

* lint: solidity lint config updated to ignore constructors

* docs(readme): updated

* lint(*.ts): fixes

* fix: accidental change

* chore: include js files in formatting

* chore: change command name back to compile-yul

* chore: typescript rollback

* ci: test_bootloader needs linting

* lint: new files linted

* chore(0.json): code formatting

* chore: unneeded prettierignore

* docs(bootloader-test): updated to use new command

* chore: test:bootloader

* lint: markdown linting added

* chore: downgraded markdownlint to avoid dependency with unwanted license

* chore: lint:fix command added

* docs: lint fix added PR template

* lint: reverted formatting of openzeppelin contracts

* fix: yarn command fixes

* lint: openzeppelin dir ignored from formatting/linting

* lint: newline at EOF of ignore files

* feat: calculate-hashes command to detect contract changes (#37)

* feat: calculate-hashes

* fix: build-yul command updated

* chore: CI workflow renamed

* feat(calculate-hashes): "--check-only" flag added

* ci: calculate-hashes added to pipeline

* modifying hash to test calculate-hashes in CI

* Revert "modifying hash to test calculate-hashes in CI"

This reverts commit 639650b3dfb4fcc7f64e75f316aa6262976c4c3f.

* chore: bytecodeHash renamed

* chore: importing and typo

* feat: revert command renames

* chore: major calculate-hashes refactor

* ci: check hashes into separate job

* ci: yarn cacheing

* fix: absolutePath

* fix: hash updated

* fix: SHA256 hash updated

* docs: readme updated

* chore: changed hashes to array

* chore: SystemContractsHashes updated

* lint(calculate-hashes): format+lint

* docs: command name typo

* fix: calculate hashes updated

* chore: automatic contracts details generation

* chore: changed the order of json properties

* feat: use boojum-integration branch of in-memory node for testing CI (matter-labs#43)

* ci: using boojum branch of test node

* test: reenable temporarily disabled tests

* ci: test node in background

* ci: caching for era-test-node

* chore: downgrading hardhat version to fix test execution

* ci: ci to run on dev and main push

* chore: set hardhat to fix v2.16.0

* ci: print era_test_node logs

* ci: change tag to commit SHA of dependency

* ci: use era-test-node-action for the testing CI (matter-labs#50)

* ci: using era-test-node-action

* ci: use boojum release of era-test-node

* ci: releaseTag fix

* ci: fix releaseTag

* ci: era-test-node-action v0.1.3

* updated hh version and solidity version (matter-labs#52)

* updated hh version and solidity version

* removed carrot

* formatting

* fixed compiler versions

* updated yul compiler version

* update hash file

* changed OZ contracts back

* update hash file

* changed compiler version

* bumped utils compiler version and hashes

* Set of fixes for boojum integration (matter-labs#53)

* apply max system contracts address

* add comment

* Allow only deployments for L1->L2

* fail to publish timesstamp

* remove trailing comma

* correct require for L1Messenger

* fix eip1559

* charge correctly for the memory overhead

* check that we have enough gas for postop

* fix comment in L1Messenger

* remove redundant check

* safeAdd for refunds

* compilation fixes + EOA work correctly on delegatecall

* correctly charge for gas overhead

* ensure that upgrade tx always succeeds

* add force deploy for keccak256

* max precompile address fix

* correct refund gas for L1 gas

* fix shifting

* correct meta calculation

* nits

* prev hash

* fix some nits

* remove unneeded casting

* fix lint

* update hashes

* update hashes

* Update bootloader/bootloader.yul

Co-authored-by: Vlad Bochok <[email protected]>

* update max precompile address constant

* Only the deployer can increment the deployment nonce

* fix lint

* add some tests

---------

Co-authored-by: Vlad Bochok <[email protected]>

* chore: synchronise linting rules of repositories (matter-labs#49)

* chore: command name changes

* lint(calculate-hashes): fix

* fix: lint:md command

* chore: package.json commands alphabetical order

* lint: using @matterlabs/eslint-config-typescript and "@matterlabs/prettier-config

* style: prettier:fix

* lint: lint:fix

* Revert "lint: lint:fix"

This reverts commit 15993b2d2ddfce0d876966d170e781645ff66cf9.

* lint: eslint rules turned off

* lint: lint:fix with new rules

* chore: .eslintignore removed

* chore: create githooks to check formatting and linting  (matter-labs#56)

* chore: pre-commit and pre-push hooks added

* docs: removed yarn lint from PR template

* Revert "chore: package.json commands alphabetical order"

This reverts commit e39a52c0b764a6ef40cfdc0fded9e068cceba1ce.

* fix hardhat

* fmt

* ignore invalid field

* Allow ts-ignore (matter-labs#59)

allow ts ignore

* nits + use the same config as on L1

* update hashes

* update hashes

* Use compatible error codes with the previous version (matter-labs#64)

* use compatible error codes with the previous version

* update hashes

* chore: normalise file path (#18)

refactor: normalize file path

Co-authored-by: Bence Haromi <[email protected]>

* ci: label-external-contributions workflow added

* ci: extension changed to yaml

* make scripts work for upgrade

* docs(readme): update zksync-era link (matter-labs#48)

docs: update docs

* docs: add Mirror link (matter-labs#51)

feat(docs): Add Mirror hyperlink

* docs: fix Discord link (matter-labs#55)

Update README.md - Fix Discord Link

Co-authored-by: Bence Haromi <[email protected]>

* docs: zk credo added

* remove admin and use governance owner as admin instead (matter-labs#85)

* correct todo

* fix lint

* fix system context

* upd bootloader hash

* ci: add workflow to label external-contributions (matter-labs#91)

* chore: moved files into system folder

* Remove allow list (matter-labs#77)

Co-authored-by: Stanislav Breadless <[email protected]>

* Upgrade zksolc version to 1.3.17 (matter-labs#97)

* Fix bridge upgrade script (matter-labs#103)

* Disallow L2 weth upgrade (matter-labs#107)

* Testing infrastructure improvements (matter-labs#82)

* System contracts test preprocessing mode

* Mock dependencies, event writer asm contract test, refactoring

* lint fix

* Small refactoring

* Change approach to use the test node

* Add docs, comments

* lint readme

* Fix hashes

* Regenerate yarn.lock to fix lints

* lint:fix

* Fix lints

* Restore lost tests

* Fix lints

* Restore yarn.lock from dev

* Update caches in workflows

* Try to disable lint cache

* Restore lint cache

* Cache contracts-preprocessed

* try to debug lint

* Regenerate yarn.lock from dev

* Restore correct deps

* Update lock

* Proposed improvements/fixes

* Use fast-glob instead glob

* Update bootloader_test artifact path

* Proposed improvements, update hashes

* Implement some fixes and improvements

* Fix lints

* Update zksync-era in bootloader tests

* Fix imports

Signed-off-by: Danil <[email protected]>

* Update contracts/test-contracts/MockContract.sol

Co-authored-by: Vlad Bochok <[email protected]>

* Fix test infra

* data -> input mock contract

* Update SC hashes

* Update zksync-era in bootloader/test_infra

* Update again

---------

Signed-off-by: Danil <[email protected]>
Co-authored-by: Danil <[email protected]>
Co-authored-by: Vlad Bochok <[email protected]>

* Scripts for governance (matter-labs#92)

Co-authored-by: Vlad Bochok <[email protected]>

* chore: merge contracts and system-contracts repos (matter-labs#98)

Co-authored-by: Stanislav Bezkorovainyi <[email protected]>
Co-authored-by: Vlad Bochok <[email protected]>

* chore: fixed migrate-governance file path

* chore: removed process.ts

* chore: added era_test_node.log to gitignore

* sync with main (matter-labs#116)

Co-authored-by: Shahar Kaminsky <[email protected]>
Co-authored-by: Maksym <[email protected]>
Co-authored-by: Pascal Marco Caversaccio <[email protected]>
Co-authored-by: Igor Aleksanov <[email protected]>
Co-authored-by: Marcin M <[email protected]>
Co-authored-by: Dennis <[email protected]>
Co-authored-by: Yury Akudovich <[email protected]>
Co-authored-by: Stanislav Bezkorovainyi <[email protected]>
Co-authored-by: Vlad Bochok <[email protected]>
Co-authored-by: koloz193 <[email protected]>
Co-authored-by: AntonD3 <[email protected]>
Co-authored-by: Jack <[email protected]>
Co-authored-by: DKlupov <[email protected]>
Co-authored-by: Salad <[email protected]>
Co-authored-by: MartinKong1990 <[email protected]>

* Revert "sync with main (matter-labs#116)" (matter-labs#117)

* ci: system-contracts-ci removed not needed caches

* AllowList removal upgrade preparation

* remove remnants of the allowlist

* rename file

* Update zksolc and ecrecover pricing

* fix typescript

* feat(tests): moved Merkle tests to foundry (matter-labs#132)

* feat(tests): migrated verifier tests to foundry (matter-labs#134)

* chore(tests): Moved priority queue tests from hardhat to foundry (matter-labs#135)

* chore(test): Moved transaction validator tests to foundry (matter-labs#151)

* test: unchecked math test (matter-labs#147)

* L2EthToken Tests (matter-labs#152)

Co-authored-by: Uacias <[email protected]>

* ci: prepare workflow for release contracts (matter-labs#163)

* ci: prepare workflow for release contracts

* Fix lint in the yaml file (matter-labs#166)

---------

Signed-off-by: Danil <[email protected]>
Co-authored-by: Shahar Kaminsky <[email protected]>
Co-authored-by: Maksym <[email protected]>
Co-authored-by: Pascal Marco Caversaccio <[email protected]>
Co-authored-by: Igor Aleksanov <[email protected]>
Co-authored-by: Marcin M <[email protected]>
Co-authored-by: Dennis <[email protected]>
Co-authored-by: Yury Akudovich <[email protected]>
Co-authored-by: Bence Haromi <[email protected]>
Co-authored-by: Stanislav Bezkorovainyi <[email protected]>
Co-authored-by: Vlad Bochok <[email protected]>
Co-authored-by: koloz193 <[email protected]>
Co-authored-by: AntonD3 <[email protected]>
Co-authored-by: Jack <[email protected]>
Co-authored-by: Bence Haromi <[email protected]>
Co-authored-by: DKlupov <[email protected]>
Co-authored-by: Salad <[email protected]>
Co-authored-by: MartinKong1990 <[email protected]>
Co-authored-by: Thomas Nguy <[email protected]>
Co-authored-by: Danil <[email protected]>
Co-authored-by: Neo <[email protected]>
Co-authored-by: Uacias <[email protected]>
koloz193 pushed a commit that referenced this issue Dec 11, 2024
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

No branches or pull requests

5 participants