Ml/hzn 2809/erc20 support by MikelHorizenLabs · Pull Request #121 · HorizenOfficial/vela

MikelHorizenLabs · 2026-03-13T11:45:50Z

Only focus in the design doc for now:

docs/design/ERC20_DEPOSITS_WITHDRAWALS_DESIGN.md

saratnt · 2026-03-13T16:11:39Z

ERC20_DEPOSITS_WITHDRAWALS_DESIGN.md

+
+The special case `tokenAddress = 0x0` preserves ETH as a first-class supported asset without needing a separate model.
+
+Each app defines an allowlist of supported tokens. That allowlist is declared in app configuration or deploy metadata and is also enforced by the application logic at runtime. In addition, the contract maintains a global token allowlist so it can reject unsupported assets before custodying them. This allows some apps to stay ETH-only while others support ETH plus selected ERC-20 tokens.


For the global allowlist, could you describe how (or if) it can be updated? Especially, is it allowed to remove tokens from the allowList? And what happens in that case to the tokens inside the apps?

Waiting for the other comment disccussion before thinking about this.

saratnt · 2026-03-13T16:31:27Z

ERC20_DEPOSITS_WITHDRAWALS_DESIGN.md

+
+The special case `tokenAddress = 0x0` preserves ETH as a first-class supported asset without needing a separate model.
+
+Each app defines an allowlist of supported tokens. That allowlist is declared in app configuration or deploy metadata and is also enforced by the application logic at runtime. In addition, the contract maintains a global token allowlist so it can reject unsupported assets before custodying them. This allows some apps to stay ETH-only while others support ETH plus selected ERC-20 tokens.


Why do we need to keep the app allowlist? Isn't the app in charge to decide which tokens can be accepted? In addition, putting the allowlist on chain would make it unmutable, while if it is just inside the wasm, the wasm could implement functions for modifying it.
The only reason I can see to put the contract in charge of checking the allowed tokens for the app is if we want to reduce the possibility to burn tokens, but this possibility is not eliminated, just reduced.

good observation.
For global allowlist I would remove it completely: I don't see good reasons to add global constraints, we can support any token.
For app allowlist I see 3 different paths:

app allowlist in the contract => we can prevent burn by mistake (or better: unlimited locking), management more complicated
app allowlist managed by the exectuor and saved in the appData (the part of the app state not directly managed by the wasm - where we also save the p521 keys) => executor could do all the checks when a deposit happens before calling the wasm, wasm part simplified but wasm can't change it (only decided at deploy time)
app allowlist saved and managed by the wasm => any request is forwarded, full control on the wasm
Maybe Mikel you could also talk with @domenicohorizen : it's also a product requirement in my opinion

Are we sure we are going to allow any token? I think it could break a lot of things. Think about rebase tokens, tokens with fees on transfer, etc etc.

not sure about that: can you highlight the problems? maybe I'm missing something but for us is just important that the token implements the ERC-20 approve + transferFrom => we just need to be able to lock & unlock tokens to the smart contract

paolocappelletti · 2026-03-27T10:58:52Z

docs/design/ERC20_DEPOSITS_WITHDRAWALS_DESIGN.md

+
+### R5 — Custody Accounting
+
+The contract tracks per-app, per-token custody via `appCustody[applicationId][tokenAddress]` and a global aggregate `totalAppCustody[tokenAddress]`. Deposits increase these balances. Withdrawals and failed-request refunds decrease them (moving value into pending claims).


the map totalAppCustody can be omitted.
for ETH (0x0) is equals to: address(this).balance
for other tokens can be obrained from: IERC20(token).balanceOf(contract)

They are different: totalAppCustody is the amount of the token still inside all the apps, while the balance contains also the claimable amounts, eg balance = totalAppCustody + totalPendingClaims

paolocappelletti · 2026-03-27T11:03:22Z

docs/design/ERC20_DEPOSITS_WITHDRAWALS_DESIGN.md

+Solvency is validated per asset, not as a single aggregated amount. The contract must verify at `stateUpdate` time that:
+- For ERC-20 tokens, `IERC20(token).balanceOf(contract) - totalAppCustody[token] - totalPendingClaims[token] >= withdrawalAmount/refund` and `address(this).balance - totalPendingClaims[0x0] - totalAppCustody[0x0] >= maxFeeValue` for fee payment and fee refund.
+- For ETH, `address(this).balance - totalPendingClaims[0x0] - totalAppCustody[0x0] >= withdrawalAmount/refund + maxFeeValue`
+App-scoped custody is checked before crediting pending claims during state updates.


checks looks wrong: the totalAppCustody element must be removed

I have rewrite it to make it clearer

docs/design/ERC20_DEPOSITS_WITHDRAWALS_DESIGN.md

andreanistico · 2026-03-27T12:37:17Z

docs/design/ERC20_DEPOSITS_WITHDRAWALS_DESIGN.md

+
+### R4 — Deposit Flow
+
+For ETH requests, `msg.value` equals `assetAmount + maxFeeValue`, matching the current behavior. For ERC-20 requests, `msg.value` equals `maxFeeValue` exactly (see 9), and the contract pulls `assetAmount` via `safeTransferFrom` (see 5). The contract verifies the actual received amount matches `assetAmount` using a balance-before/after check (see 2).


Here I would specify that the ProcessorEndpoint should be approved to spend user's tokens before the user sends a request

andreanistico · 2026-03-27T12:45:44Z

docs/design/ERC20_DEPOSITS_WITHDRAWALS_DESIGN.md

+- **ERC-777**: must be excluded from the allowlist (see 3, R2). `nonReentrant` provides backup defense (R10).
+- **Non-standard return values**: handled transparently by `SafeERC20` (see 5, R4).
+
+### R15 — Admin Surplus Recovery (Optional, Deferred)


Maybe it is something that could be considered later, but it would be safer if the tokens managed by ProcessorEndpoint would be owned by another separated contract, to separate the logic of token management from the rest of the processor.
For now it can be developed as a separated contract that is then inherited by ProcessorEndpoint, so they will be the same contract but at least the code and logic will be separated

Ok, I wouldn't change the document for this, but it is something to take into consideration during the implementation

MikelHorizenLabs added 2 commits March 13, 2026 09:54

docs: first version of the ERC20 support design doc

d127751

docs: adding more details to ERC20 deposit design doc

737b438

MikelHorizenLabs marked this pull request as ready for review March 13, 2026 11:45

MikelHorizenLabs requested review from paolocappelletti and saratnt March 13, 2026 11:46

saratnt requested changes Mar 13, 2026

View reviewed changes

MikelHorizenLabs and others added 3 commits March 16, 2026 11:16

docs: app scoped custody added to the design doc

9ba796c

Merge branch 'dev' into ml/HZN-2809/erc20_support

09633da

Updated ERC20 design doc

f5a9e7b

saratnt assigned alsala and andreanistico and unassigned andreanistico and alsala Mar 27, 2026

saratnt requested review from alsala and andreanistico March 27, 2026 10:21

Moved doc under docs/design

bbda3c1

paolocappelletti requested changes Mar 27, 2026

View reviewed changes

Fixes after review

0a5a20a

saratnt requested a review from paolocappelletti March 27, 2026 11:51

andreanistico reviewed Mar 27, 2026

View reviewed changes

paolocappelletti approved these changes Mar 27, 2026

View reviewed changes

Fix after review

f863b8f

saratnt requested a review from andreanistico March 27, 2026 14:15


		The special case `tokenAddress = 0x0` preserves ETH as a first-class supported asset without needing a separate model.

		Each app defines an allowlist of supported tokens. That allowlist is declared in app configuration or deploy metadata and is also enforced by the application logic at runtime. In addition, the contract maintains a global token allowlist so it can reject unsupported assets before custodying them. This allows some apps to stay ETH-only while others support ETH plus selected ERC-20 tokens.


		### R5 — Custody Accounting

		The contract tracks per-app, per-token custody via `appCustody[applicationId][tokenAddress]` and a global aggregate `totalAppCustody[tokenAddress]`. Deposits increase these balances. Withdrawals and failed-request refunds decrease them (moving value into pending claims).


		### R4 — Deposit Flow

		For ETH requests, `msg.value` equals `assetAmount + maxFeeValue`, matching the current behavior. For ERC-20 requests, `msg.value` equals `maxFeeValue` exactly (see 9), and the contract pulls `assetAmount` via `safeTransferFrom` (see 5). The contract verifies the actual received amount matches `assetAmount` using a balance-before/after check (see 2).

Conversation

MikelHorizenLabs commented Mar 13, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paolocappelletti Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

paolocappelletti Mar 16, 2026 •

edited

Loading