Arbitrum L1 to L2 messaging proposal #13516

blahkheart · 2024-03-25T14:37:42Z

Description

This PR adds an example of how to create a DAO proposal for execution of Arbitrum's L1-to-L2 message passing system (aka "retryable tickets")

Issues

Fixes #13515
Refs #13515

Checklist:

1 PR, 1 purpose: my Pull Request applies to a single purpose
I have commented my code, particularly in hard-to-understand areas
I have updated the docs to reflect my changes if applicable
I have added tests (and stories for frontend components) that prove my fix is effective or that my feature works
I have performed a self-review of my own code
If my code involves visual changes, I am adding applicable screenshots to this thread

Release Note Draft Snippet

cla-bot · 2024-03-25T14:37:45Z

Thank you for your pull request and welcome to Unlock! We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @blahkheart on file.
In order for us to review and merge your code, please open another pull request with a single modification: your github username added to the file .clabot.
Thank you!

cla-bot · 2024-03-25T14:42:36Z

Thank you for your pull request and welcome to Unlock! We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @blahkheart on file.
In order for us to review and merge your code, please open another pull request with a single modification: your github username added to the file .clabot.
Thank you!

clemsos

Thanks for the PR. Good start. A few comments added !

.clabot

governance/.env

clemsos · 2024-03-25T19:11:52Z

governance/.env

+# Duplicate this file as .env
+
+# Your Private key
+DEVNET_PRIVKEY="0x14e1a3103ce06d0348acf19df3df1965ee55f9cdb16708c0bb8cd3d843ab8805"


You should never check a private key into a git repo...

Yeah totally agree with that, I didn't know .env wasn't included in the .gitignore file, especially because I saw a .env.copy file already present in the /governance/ folder, which I copied and renamed to .env. For what it's worth it's a fresh wallet I created specifically for the tests.

governance/.env

governance/proposals/010-arbitrum-l1-l2-messaging.js

cla-bot · 2024-03-26T11:04:56Z

Thank you for your pull request and welcome to Unlock! We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @blahkheart on file.
In order for us to review and merge your code, please open another pull request with a single modification: your github username added to the file .clabot.
Thank you!

clemsos · 2024-03-26T11:57:05Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+const l2Wallet = new ethers.Wallet(walletPrivateKey, l2Provider)
+
+module.exports = async () => {
+  await arbLog('Cross-chain Proposer')


what is arbLog used for ?

It came with the arbitrum sdk, its more or less a branding/description of the script.

Can we just use a regular console.log please?

clemsos · 2024-03-26T11:59:12Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+        gasPriceBid,
+        transfer_calldata,
+      ],
+      value: L1ToL2MessageGasParams.deposit.toNumber() * 10, // I Multiply by 10 to add extra in case gas changes due to proposal delay


Are you sure you need to pass the value again here? It is already declared above

Yes, from my understanding of the existing format, and correct me if I'm wrong but this value property is responsible for passing the eth amount to a payable function, and in this case that's the eth deposit to be sent to arbitrum's inbox contract.
If you were referring to this line const values = [L1ToL2MessageGasParams.deposit.toNumber() * 10] that was declared to show how the proposal can be created from an interface like etherscan or remix, but if its not necessary or confusing I can remove it as well.

julien51

Can you please use #13509 to run tests for your proposal, then import the values in Tenderly to check that it passes?

julien51 · 2024-03-26T12:26:38Z

governance/.env.copy

+export GNOSISSCAN_API_KEY=
+
+# This is a sample .env file for use in local development.


Please remeove these changes...

Okay got it

julien51 · 2024-03-26T12:27:09Z

governance/package.json

+    "@arbitrum/nitro-contracts": "v1.0.2",
+    "@arbitrum/sdk": "^v3.1.9",


Please use fixed version numbers (no v or ^)

julien51 · 2024-03-26T12:27:16Z

governance/package.json

@@ -21,8 +23,9 @@
    "@unlock-protocol/hardhat-helpers": "workspace:^",
    "@unlock-protocol/hardhat-plugin": "workspace:^",
    "@unlock-protocol/networks": "workspace:./packages/networks",
+    "arb-shared-dependencies": "^1.0.0",


julien51 · 2024-03-26T12:28:05Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+/**
+ * Set up: instantiate L1 / L2 wallets connected to providers
+ */
+const walletPrivateKey = 'Your_Private_key'


Please use env variables here

@clemsos earlier informed me .env is not supported, how else would I use env variables?

Process.env.PRIVATE_KEY for example

julien51 · 2024-03-26T12:28:34Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+ * Set up: instantiate L1 / L2 wallets connected to providers
+ */
+const walletPrivateKey = 'Your_Private_key'
+const L1RPC = 'https://mainnet.infura.io/v3/<your_infura_api_key>'


Please use https://rpc.unlock-protocol.com/1 so that this does not break in the future.

julien51 · 2024-03-26T12:31:02Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+  const ARBTokenAddressOnL2 = '0x912CE59144191C1204E64559FE8253a0e49E6548' // ARB TOKEN ADDRESS ON ARBITRUM ONE
+  const grantsContractAddress = '0x00D5E0d31d37cc13C645D86410aB4cB7Cb428ccA'
+  const timelockL2Alias = '0x28ffDfB0A6e6E06E95B3A1f928Dc4024240bD87c' // Timelock Alias Address on L2
+  const L1TimelockContract = '0x17EEDFb0a6E6e06E95B3A1F928dc4024240BC76B' // Timelock Address mainnet


We should probably move that to a config file in this folder.

julien51 · 2024-03-26T12:32:34Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+      from: L1TimelockContract,
+      to: ARBTokenAddressOnL2,
+      l2CallValue: 0,
+      excessFeeRefundAddress: l2Wallet.address,


Can you explain why l2Wallet would recive the excess here?

That's following the format from arbitrum's example, but I didn't need to change it because the L1 and L2 wallets are the same. Ideally you want to specify the wallet address you want the excess eth refund to be sent to if it's different.

Where is the excess ETH coming from? An funds should go back to the DAO and not to some external address.

Got it, it has now been changed to the DAO's address. The reason for having it that way in the first place is due to the nature of the transaction. The createRetryable function forces the caller to pay the value sent to the Delayed Inbox contract, Therefore ETH is paid by the person calling the execute() function and not the Timelock. So it was set up so whoever creates the proposal and provides the private key gets any excess ETH refund sent to that address. But that was flawed by the assumption that the proposer will always be the executor. Ideally, the DAO will fund/reimburse whoever executes the proposal so excess should return to the DAO, thank you for pointing that out.

julien51 · 2024-03-26T12:32:43Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+      to: ARBTokenAddressOnL2,
+      l2CallValue: 0,
+      excessFeeRefundAddress: l2Wallet.address,
+      callValueRefundAddress: l2Wallet.address,


Same reason as before

julien51 · 2024-03-26T12:33:19Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+  const inbox_calldata = iface_inbox.encodeFunctionData(
+    'createRetryableTicket',
+    [
+      ARBTokenAddressOnL2,


Please add comments in front of each of these params to provide their names

julien51 · 2024-03-26T12:35:35Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+    ]
+  )
+
+  const proposalName = `# Test Transaction before 7k ARB Transfer To Fund Unlock Protocol’s Ecosystem via Grants Stack  This proposal requests to use 1 ARB from the tokens given to Unlock Protocol DAO by ArbitrumDAO to run a test transaction to de-risk the transfer of 7k ARB tokens to fund the retroQF round on Grants Stack.`


Please format this correctly.

This would be

Test Transaction before 7k ARB Transfer To Fund Unlock Protocol’s Ecosystem via Grants Stack This proposal requests to use 1 ARB from the tokens given to Unlock Protocol DAO by ArbitrumDAO to run a test transaction to de-risk the transfer of 7k ARB tokens to fund the retroQF round on Grants Stack.

Make sure there is a title, one (or more) distinct paragraphs, as well as links to provide clarity (links to the snapshot, to the UI on which people are voting... etc)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

…rk-parse to v11 (unlock-protocol#13531) * Updated unified & related dependencies * Updated remark

…15, remark-parse to v11" (unlock-protocol#13532) Revert "fix(deps): update dependency unified to v11, remark-html to v15, rema…" This reverts commit 80969a6.

unlock-protocol#13528) * changed the UI for refunds per Gnosis * refactored receipt number

* Update Referrals.tsx microcopy fix * Update CancellationForm.tsx microcopy update * Update UpdateReferralFee.tsx microcopy update

…tocol#13534) * add publicLock changelog * add Unlock changelog

…-protocol#13509) * allow test execution of proposal from tx id * parse and pass correctly tx args * add unlock addresses in whales * allow proposal id only for votes * pass odwn tx receipt * herlpes for gov contract * log events after proposal execution * Update governance/scripts/gov/index.js --------- Co-authored-by: Julien Genestoux <julien.genestoux@gmail.com>

…3541) * Updated nuintun/qrcode * Update qrcode.ts

julien51 · 2024-04-02T18:07:44Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+ */
+const walletPrivateKey = process.env.PRIVATE_KEY
+const L1RPC = 'https://rpc.unlock-protocol.com/1' // mainnet RPC
+const L2RPC = 'https://rpc.unlock-protocol.com/42161' // Arbitrum RPC


julien51 · 2024-04-02T18:12:52Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+const l2Wallet = new ethers.Wallet(walletPrivateKey, l2Provider)
+
+module.exports = async () => {
+  console.log('Cross-chain Proposer')


This is misleading. Can you use a better description please?

I'm not sure what you mean here, what are you referring to please?

julien51 · 2024-04-02T18:34:12Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+  const tokenAmount = ethers.parseEther('1')
+
+  // Create an instance of the Interface from the ABIs
+  const iface_erc20 = new ethers.Interface(ERC20_ABI)


Please respect the styleguide if you can:

Suggested change

const iface_erc20 = new ethers.Interface(ERC20_ABI)

const ifaceErc20 = new ethers.Interface(ERC20_ABI)

julien51 · 2024-04-02T18:34:25Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+  const iface_inbox = new ethers.Interface(INBOX_ABI)
+
+  // Encode the ERC20 Token transfer calldata
+  const transfer_calldata = iface_erc20.encodeFunctionData('transfer', [


Suggested change

const transfer_calldata = iface_erc20.encodeFunctionData('transfer', [

const transferCalldata = iface_erc20.encodeFunctionData('transfer', [

julien51 · 2024-04-02T19:05:25Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+    l1Provider
+  )
+  const gasPriceBid = await l2Provider.getGasPrice()
+  const ETHDeposit = L1ToL2MessageGasParams.deposit.toNumber() * 10 // I Multiply by 10 to add extra in case gas changes due to proposal delay


Can you explain what this ETHDeposit is?

ETHDeposit is the amount ETH value that must be sent to the Delayed Inbox Contract for a successful retryable ticket creation. It is estimated by calling the l1ToL2MessageGasEstimate.estimateAll({params}) function provided by the Arbitrum sdk. When the createRetryableTicket function is called, it forces the caller to pay this amount else reverts, this is why the excessFeeRefundAddress you asked about earlier is needed, to send any excess back. Here I multiply the actual value by 10 as a buffer because in the time it takes to execute a proposal that value may change.

Note that, the function createRetryableTicket forces the sender to provide a reasonable amount of funds (at least enough for submitting, and attempting to execute the ticket), but that doesn't guarantee a successful auto-redemption. Checkout arbitrum docs for more info..

ok that makes sense. Please add this nside the proposal. Because this comment won't be easy to find for people who are reviewing the proposal!

* Used providers, and addresses from networks package * Renamed config.js to constants.js * Used suggested style guide for variable names * More detailed description

julien51 · 2024-04-03T12:29:24Z

governance/.gitignore

@@ -2,6 +2,7 @@
 cache
 contracts
 artifacts
+.env


julien51 · 2024-04-03T12:30:40Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+  ).connect(l2Wallet)
+
+  const balanceOf = await L2TokenContract.balanceOf(TIMELOCK_L2_ALIAS)
+  const tokenAmount = ethers.parseEther('1')


Please let's use the real anount. It does not make sense to send 1 ARB if this has been tested correctly (as I am sure you have done by now!)

Also, please use ethers.parseUnits as this is an ERC20, not Ethers.

julien51 · 2024-04-03T12:32:10Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+   This proposal requests to use 1 ARB from the tokens given to Unlock Protocol DAO by ArbitrumDAO to run a test transaction to de-risk the transfer of 7k ARB tokens to fund the retroQF round on Grants Stack.
+
+  #### Current situation of DAO's ARB Tokens
+    - total: ${ethers.formatEther(balanceOf).toString()} ARB.


same here. do not use formatEther when it is an ERC20.

julien51 · 2024-04-03T12:32:21Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+
+  #### About the proposal
+    The proposal contains a single call to the Arbitrum Delayed Inbox Contract's \`createRetryableTicket\` function on mainnet to create a \`Retryable Ticket\` that will attempt to execute an L2 request to the ARB token contract to transfer ${ethers
+      .formatEther(tokenAmount)


julien51 · 2024-04-03T12:32:24Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+    The proposal contains a single call to the Arbitrum Delayed Inbox Contract's \`createRetryableTicket\` function on mainnet to create a \`Retryable Ticket\` that will attempt to execute an L2 request to the ARB token contract to transfer ${ethers
+      .formatEther(tokenAmount)
+      .toString()} of token from the Timelock L2 Alias address \`${TIMELOCK_L2_ALIAS}\` to the [grants contract](https://arbiscan.io/address/0x00d5e0d31d37cc13c645d86410ab4cb7cb428cca) - \`transfer(${GRANTS_CONTRACT_ADDRESS},${ethers
+      .formatEther(tokenAmount)


julien51

since the DAO does not hold any ETH and yet it needs to send ETH to the mailbox contract (if I understand your proposal correctly), how will this work?

governance/proposals/010-arbitrum-l1-l2-messaging.js

governance/proposals/constants.js

…-msg-proposal

clemsos · 2024-04-03T14:55:20Z

@blahkheart I refactored the proposal script a bit to make it clearer. Almost there !

clemsos · 2024-04-03T14:56:26Z

governance/package.json

    "eslint": "8.54.0",
    "ethers": "6.10.0",
+    "ethers5": "npm:ethers@5",


We have to add ethers5 here, as it is required by the arbitrum sdk

blahkheart · 2024-04-03T16:31:00Z

@blahkheart I refactored the proposal script a bit to make it clearer. Almost there !

Great! Is any further action required on my part?

blahkheart · 2024-04-03T17:08:33Z

since the DAO does not hold any ETH and yet it needs to send ETH to the mailbox contract (if I understand your proposal correctly), how will this work?

I think I mentioned this earlier, one way I feel this could work is whoever calls the execute function on the governor has to provide the ETH to be sent. So, the DAO then funds/refunds the person, anyone can do this (I committed to do that on the test one, and I'm happy to do so again ) or the DAO can elect someone to do that and fund/refund them to do it.

* Use actual amount of ARB tokens required

* Use correct proposal amount * Revert .gitignore, and .env.copy changes

julien51 · 2024-04-03T20:46:45Z

governance/proposals/010-arbitrum-l1-l2-messaging.js

+  # Transfer 8200 ARB To Fund Unlock Protocol’s Ecosystem via Grants Stack
+
+  ### Goal of the proposal
+   This proposal requests to use 8200 ARB from the tokens given to Unlock Protocol DAO by ArbitrumDAO to fund the retroQF round on Grants Stack for projects building on Unlock Protocol.


This is UP from what the snapshot proposal initially asked...

Asking for 7k in ARB to help Fund Unlock Protocol’s Ecosystem via Grants Stack

There is an explanation for why this is up from the initial 7k as well as a reference to the snapshot proposal included in the description

julien51 · 2024-04-03T20:47:15Z

Please @blahkheart merge the conflicts on yarn.lock!

blahkheart added 3 commits March 23, 2024 00:22

added 010-arbitrum-l1-l2-messaging.js to /goverance/proposals

f0cbaf2

removed unnecessary comments

85376b1

moved ERC20_ABI and INBOX_ABI inline and removed /helper/abi folder

247edf0

signed Contributor License Agreement

2f0608f

clemsos requested changes Mar 25, 2024

View reviewed changes

removed console logs, and .env file

b9c3467

clemsos reviewed Mar 26, 2024

View reviewed changes

Merge branch 'master' into l1-l2-msg-proposal

20939ee

cla-bot bot added the cla-signed label Mar 26, 2024

julien51 requested changes Mar 26, 2024

View reviewed changes

blahkheart and others added 16 commits March 30, 2024 18:32

used fixed version numbers for arbitrum packages, added config.js file

506fdc6

changed excessFeeRefundAddress to the dao's address

09aada2

added comments in front of params to provide their names

6cd79c2

formated proposal description correctly

cd41637

feat(locksmith): better fatal error handling with a logging statement

80b6fe6

feat(locksmith): adding cache for decoy users

14273ca

fix(deps): update dependency express [security] (unlock-protocol#13524)

6709e11

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

fix(deps): update dependency unified to v11, remark-html to v15, rema…

a6942e4

…rk-parse to v11 (unlock-protocol#13531) * Updated unified & related dependencies * Updated remark

Revert "fix(deps): update dependency unified to v11, remark-html to v…

b54ffa4

…15, remark-parse to v11" (unlock-protocol#13532) Revert "fix(deps): update dependency unified to v11, remark-html to v15, rema…" This reverts commit 80969a6.

feat(unlock-app): changed the UI for refunds per Dappcon Team requests (

347d6bf

unlock-protocol#13528) * changed the UI for refunds per Gnosis * refactored receipt number

fix(unlock-protocol-com): removed distDir from next config

98935ee

Ccarfi microcopy fixes on referrals and refunds (unlock-protocol#13536)

e98703b

* Update Referrals.tsx microcopy fix * Update CancellationForm.tsx microcopy update * Update UpdateReferralFee.tsx microcopy update

feat(docs): add changelog for Unlock 13 and PublicLock 14 (unlock-pro…

6c9f33a

…tocol#13534) * add publicLock changelog * add Unlock changelog

add contributors to .clabot (unlock-protocol#13537)

3e817b7

fix(deps): update dependency @nuintun/qrcode to v4 (unlock-protocol#1…

24d81c3

…3541) * Updated nuintun/qrcode * Update qrcode.ts

julien51 reviewed Apr 2, 2024

View reviewed changes

julien51 requested changes Apr 2, 2024

View reviewed changes

Updated PR:

c9a28c1

* Used providers, and addresses from networks package * Renamed config.js to constants.js * Used suggested style guide for variable names * More detailed description

blahkheart requested review from clemsos and julien51 April 3, 2024 09:41

julien51 reviewed Apr 3, 2024

View reviewed changes

governance/.gitignore Outdated

@@ -2,6 +2,7 @@

cache

contracts

artifacts

.env

Copy link

Member

julien51 Apr 3, 2024

Choose a reason for hiding this comment

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

same here

julien51 requested changes Apr 3, 2024

View reviewed changes

julien51 reviewed Apr 3, 2024

View reviewed changes

clemsos reviewed Apr 3, 2024

View reviewed changes

governance/proposals/010-arbitrum-l1-l2-messaging.js Outdated Show resolved Hide resolved

clemsos reviewed Apr 3, 2024

View reviewed changes

governance/proposals/constants.js Outdated Show resolved Hide resolved

clemsos and others added 5 commits April 3, 2024 15:05

Update governance/proposals/constants.js

4d42172

Update governance/proposals/010-arbitrum-l1-l2-messaging.js

c33c977

use ethers5

79327c5

refactor proposal

ace2d13

Merge branch 'master' of github.com:unlock-protocol/unlock into l1-l2…

794914d

…-msg-proposal

clemsos reviewed Apr 3, 2024

View reviewed changes

blahkheart added 2 commits April 3, 2024 20:58

* Revert change to .gitignore, and .env.copy

a82a7c0

* Use actual amount of ARB tokens required

* Update proposal description

debbbdf

* Use correct proposal amount * Revert .gitignore, and .env.copy changes

blahkheart requested a review from julien51 April 3, 2024 19:59

julien51 reviewed Apr 3, 2024

View reviewed changes

Resolved yarn.lock conflict

6687533

julien51 approved these changes Apr 4, 2024

View reviewed changes

julien51 merged commit a5c3404 into unlock-protocol:master Apr 4, 2024
9 checks passed

		export GNOSISSCAN_API_KEY=

		# This is a sample .env file for use in local development.

		"@arbitrum/nitro-contracts": "v1.0.2",
		"@arbitrum/sdk": "^v3.1.9",

	const iface_erc20 = new ethers.Interface(ERC20_ABI)
	const ifaceErc20 = new ethers.Interface(ERC20_ABI)

	const transfer_calldata = iface_erc20.encodeFunctionData('transfer', [
	const transferCalldata = iface_erc20.encodeFunctionData('transfer', [

Arbitrum L1 to L2 messaging proposal #13516

Arbitrum L1 to L2 messaging proposal #13516

Conversation

blahkheart commented Mar 25, 2024

Description

Issues

Checklist:

Release Note Draft Snippet

cla-bot bot commented Mar 25, 2024

cla-bot bot commented Mar 25, 2024

clemsos left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-bot bot commented Mar 26, 2024

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

julien51 left a 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

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

blahkheart Apr 3, 2024 • edited

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

julien51 left a comment

Choose a reason for hiding this comment

clemsos commented Apr 3, 2024

clemsos Apr 3, 2024 • edited

Choose a reason for hiding this comment

blahkheart commented Apr 3, 2024

blahkheart commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien51 commented Apr 3, 2024

clemsos left a comment •

edited

blahkheart Apr 3, 2024 •

edited

clemsos Apr 3, 2024 •

edited