implement `AstriaMintableERC20`, update deposit tx to support ERC20 mints #20

noot · 2024-05-22T23:41:25Z

implement AstriaMintableERC20.sol, allows an immutable address bridge to mint or burn
this address should be set at genesis and the sender of DepositTxs is set to this address
DepositTx was updated to include to and data which are set for erc20 mints (later on burns), to is set to the erc20 address of the AstriaMintableERC20 and data is the calldata to call the mint function
genesis config fields for erc20s must be set to allow bridging erc20s from the sequencer. namely the erc20 contract address must be set, meaning the token must already be deployed

testing:

i tested this manually end-to-end using the instructions in contracts/README.md.

closes #9

…ptions

v1.8.2

…c20-deposit

Lilyjjo · 2024-05-31T10:37:24Z

grpc/execution/server.go

+					//
+					// the fees are spent from the "bridge account" which is not actually a real account, but is instead some
+					// address defined by consensus, so the gas cost is not actually deducted from any account.
+					Gas:  16000,


I'm not sure if this is a big enough number, the gas costs of a tx is normally gasUnits * gasPrice. I'd either make it so the DepositTxs pay no gas or set this to something crazy large to account for EIP1559 gas price swings

I originally tried to just make the tx not use gas, but it got a bit janky when it came to the state transition gas accounting D:

the gas parameter here is just for the gas units, not the gas price, and I don't think gas itself can vary that much? DepositTxs don't have a gas price since they're inherent to consensus

Lol I got that wrong, you're right it's the gas unit and not gas cost. This should be fine then

Lilyjjo · 2024-05-31T11:47:22Z

contracts/src/Counter.sol

@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: UNLICENSED


This file can be deleted

Lilyjjo · 2024-06-03T06:28:22Z

grpc/execution/server.go

 			assetID := [32]byte{}
 			copy(assetID[:], deposit.AssetId[:32])
+
 			if _, ok := s.bridgeAllowedAssetIDs[assetID]; !ok {


We don't check that the bridge account itself is tied to this asset or if the bridge's start height requirement has been met. Might be safer to make these checks

good catch, added!

## Summary implement withdrawals of ERC20 tokens that are of type `AstriaBridgeableERC20` (see implemented contract). ## Background we want to be able to withdraw ERC20 tokens that are bridged to a rollup. ## Changes - implement `AstriaBridgeableERC20` which is a standard `ERC20` contract with additional functionality for minting (not used by the withdrawer, implemented/tested here astriaorg/astria-geth#20) as well as functionality for withdrawing - implement `IAstriaWithdrawer` which is implemented by both `AstriaWithdrawer` and `AstriaMintableERC20`. - the withdrawer now interacts with a `IAstriaWithdrawer`. both native assets and ERC20 withdrawals have the same event signatures, so no additional code was needed for the withdrawer itself. - to use the withdrawer with an `AstriaBridgeableERC20`, the `ASTRIA_BRIDGE_WITHDRAWER_ETHEREUM_CONTRACT_ADDRESS` is set to some `AstriaMintableERC20`, and `ASTRIA_BRIDGE_WITHDRAWER_ROLLUP_ASSET_DENOMINATION` is set to the rollup asset's denomination as represented on the sequencer. for example, if the asset is represented on the sequencer is `transfer/channel-1/usdc`, that is the rollup asset denomination. the `name/symbol` of the ERC20 contract are not relevant. - also update the build script to write the generated abigen contract bindings to files, this is easier for debugging and unit testing. ## Testing unit tests ## Related Issues closes #924

joroshiba · 2024-06-08T18:46:19Z

contracts/astria_bridgeable_erc20.go

can we add a .gitattributes file and mark as generated?

joroshiba · 2024-06-08T18:48:09Z

core/types/deposit_tx.go

 func (tx *DepositTx) gas() uint64            { return tx.Gas }
 func (tx *DepositTx) gasFeeCap() *big.Int    { return new(big.Int) }
 func (tx *DepositTx) gasTipCap() *big.Int    { return new(big.Int) }
 func (tx *DepositTx) gasPrice() *big.Int     { return new(big.Int) }
 func (tx *DepositTx) value() *big.Int        { return tx.Value }
 func (tx *DepositTx) nonce() uint64          { return 0 }
-func (tx *DepositTx) to() *common.Address    { return nil }
+func (tx *DepositTx) to() *common.Address    { return tx.To }


how were we identifying the correct account to send funds to previously?

using the From field, but i felt that was confusing, and now we need to set the From field to the bridge "account"

joroshiba · 2024-06-08T18:53:08Z

grpc/execution/server.go

+			if cfg.Erc20Asset == nil && nativeBridgeSeen {
 				return nil, errors.New("only one native bridge address is allowed")
 			}
-			if cfg.Erc20Asset != nil && !nativeBridgeSeen {
+			if cfg.Erc20Asset == nil && !nativeBridgeSeen {
 				nativeBridgeSeen = true
 			}


I know I wrote this but maybe a slightly cleaner if we combine?

if cfg.Erc20Asset == nil { if nativeBridgeSeen { return nil, errors.New("only one native bridge address is allowed") } nativeBridgeSeen = true }

joroshiba · 2024-06-08T18:55:12Z

grpc/execution/server.go

 				nativeBridgeSeen = true
 			}

+			if cfg.Erc20Asset != nil && bc.Config().AstriaBridgeSenderAddress == (common.Address{}) {
+				return nil, errors.New("astria bridge sender address must be set for bridged ERC20 assets")


Noted further down, but we should probably use string.

AstriaBridgeSenderAddress is an evm address address (the fake address mints are sent from) so can leave this

joroshiba · 2024-06-08T18:57:25Z

params/config.go

@@ -346,6 +346,7 @@ type ChainConfig struct {
 	AstriaCelestiaInitialHeight    uint64                      `json:"astriaCelestiaInitialHeight"`
 	AstriaCelestiaHeightVariance   uint64                      `json:"astriaCelestiaHeightVariance,omitempty"`
 	AstriaBridgeAddressConfigs     []AstriaBridgeAddressConfig `json:"astriaBridgeAddresses,omitempty"`
+	AstriaBridgeSenderAddress      common.Address              `json:"astriaBridgeSenderAddress,omitempty"`


We should use a bech32m encoded string here, given that we marked the bytes as deprecated.

Does this have any implications on the contract?

same as above, this is actually supposed to be an evm address, it's not an astria address

i updated the contracts for bech32 also btw

noot added 6 commits May 22, 2024 19:25

implement AstriaMintableERC20, update deposit tx to have erc20 mint o…

64491c4

…ptions

add astriaBridgeSenderAddress to genesis

27d7b77

implement erc20 deposit state transition logic

b4ed58b

chore: forge init

d056553

forge install: forge-std

a032bfc

v1.8.2

use forge for contract dependencies

2ad8943

noot marked this pull request as ready for review May 23, 2024 22:39

noot requested review from joroshiba and mycodecrafting as code owners May 23, 2024 22:39

noot added 5 commits May 23, 2024 20:47

update genesis denom

1ef30ec

xMerge branch 'main' of github.com:astriaorg/astria-geth into noot/er…

b04a771

…c20-deposit

contract scripts and readme updates

fd9afd9

fix gas for erc20 mints

c99b5d2

add to readme and scripts

563bd33

Lilyjjo reviewed May 31, 2024

View reviewed changes

remove file

e21153d

Lilyjjo reviewed Jun 3, 2024

View reviewed changes

noot added 5 commits June 4, 2024 18:08

update contract to have WITHDRAWER address

239ff23

add SEQUENCER_ASSET_ID to contract

d6648d3

add withdrawal functions to AstriaMintableERC20

7511980

update bindings and script

f66ae30

contract updates for withdrawer interface

43fb433

noot mentioned this pull request Jun 5, 2024

feat(withdrawer): bridged ERC20 token withdrawals astriaorg/astria#1149

Merged

noot added 4 commits June 5, 2024 17:48

merge with main

f08e2e9

fix unit tests

f1ac957

fix unit tests

5d119e8

add start height and asset id/bridge addr match checks

8f27bb9

noot requested a review from Lilyjjo June 6, 2024 18:26

move contracts to submodule

c8feea2

joroshiba reviewed Jun 8, 2024

View reviewed changes

address comments, update contracts submodule

7af66e1

noot requested a review from joroshiba June 9, 2024 19:34

joroshiba approved these changes Jun 10, 2024

View reviewed changes

noot merged commit 3d160b2 into main Jun 10, 2024
2 checks passed

noot deleted the noot/erc20-deposit branch June 10, 2024 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement `AstriaMintableERC20`, update deposit tx to support ERC20 mints #20

implement `AstriaMintableERC20`, update deposit tx to support ERC20 mints #20

noot commented May 22, 2024 •

edited

Loading

Lilyjjo May 31, 2024

noot Jun 1, 2024 •

edited

Loading

Lilyjjo Jun 3, 2024

Lilyjjo May 31, 2024

Lilyjjo Jun 3, 2024

noot Jun 6, 2024

joroshiba Jun 8, 2024

joroshiba Jun 8, 2024

noot Jun 8, 2024

joroshiba Jun 8, 2024

joroshiba Jun 8, 2024

noot Jun 8, 2024

joroshiba Jun 8, 2024

noot Jun 8, 2024

noot Jun 8, 2024

implement AstriaMintableERC20, update deposit tx to support ERC20 mints #20

implement AstriaMintableERC20, update deposit tx to support ERC20 mints #20

Conversation

noot commented May 22, 2024 • edited Loading

Choose a reason for hiding this comment

noot Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

implement `AstriaMintableERC20`, update deposit tx to support ERC20 mints #20

implement `AstriaMintableERC20`, update deposit tx to support ERC20 mints #20

noot commented May 22, 2024 •

edited

Loading

noot Jun 1, 2024 •

edited

Loading