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

Arbitrum L1 to L2 messaging proposal #13516

Merged
merged 39 commits into from Apr 4, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
f0cbaf2
added 010-arbitrum-l1-l2-messaging.js to /goverance/proposals
blahkheart Mar 22, 2024
85376b1
removed unnecessary comments
blahkheart Mar 22, 2024
247edf0
moved ERC20_ABI and INBOX_ABI inline and removed /helper/abi folder
blahkheart Mar 25, 2024
2f0608f
signed Contributor License Agreement
blahkheart Mar 25, 2024
b9c3467
removed console logs, and .env file
blahkheart Mar 26, 2024
20939ee
Merge branch 'master' into l1-l2-msg-proposal
julien51 Mar 26, 2024
506fdc6
used fixed version numbers for arbitrum packages, added config.js file
blahkheart Mar 28, 2024
09aada2
changed excessFeeRefundAddress to the dao's address
blahkheart Mar 28, 2024
6cd79c2
added comments in front of params to provide their names
blahkheart Mar 28, 2024
cd41637
formated proposal description correctly
blahkheart Mar 28, 2024
80b6fe6
feat(locksmith): better fatal error handling with a logging statement
julien51 Mar 26, 2024
14273ca
feat(locksmith): adding cache for decoy users
julien51 Mar 26, 2024
6709e11
fix(deps): update dependency express [security] (#13524)
renovate[bot] Mar 27, 2024
a6942e4
fix(deps): update dependency unified to v11, remark-html to v15, rema…
SVell Mar 27, 2024
b54ffa4
Revert "fix(deps): update dependency unified to v11, remark-html to v…
julien51 Mar 27, 2024
347d6bf
feat(unlock-app): changed the UI for refunds per Dappcon Team request…
julien51 Mar 27, 2024
98935ee
fix(unlock-protocol-com): removed distDir from next config
julien51 Mar 27, 2024
e98703b
Ccarfi microcopy fixes on referrals and refunds (#13536)
ccarfi Mar 28, 2024
6c9f33a
feat(docs): add changelog for Unlock 13 and PublicLock 14 (#13534)
clemsos Mar 28, 2024
3e817b7
add contributors to .clabot (#13537)
TylerSehr Mar 28, 2024
39b4e79
feat(governance): allow test execution of proposal from tx id (#13509)
clemsos Mar 29, 2024
24d81c3
fix(deps): update dependency @nuintun/qrcode to v4 (#13541)
SVell Mar 29, 2024
eabfe4d
fix(deps): update dependency unified to v11, remark-html to v16, rema…
SVell Mar 29, 2024
cbdfb95
fix(locksmith) fixing export (#13542)
julien51 Mar 29, 2024
fa63c3d
feat(networks): add support for Base sepolia (#13529)
clemsos Mar 29, 2024
0f7ec36
cleanup: removed goerli (#13540)
julien51 Mar 30, 2024
ce0e970
Updated PR to address comments
blahkheart Mar 30, 2024
c1fd7d7
Merge branch 'master' into l1-l2-msg-proposal
clemsos Apr 1, 2024
7bced2f
Updated PR
blahkheart Apr 1, 2024
0a993b8
Resolved yarn.lock merge conflict
blahkheart Apr 2, 2024
c9a28c1
Updated PR:
blahkheart Apr 3, 2024
4d42172
Update governance/proposals/constants.js
clemsos Apr 3, 2024
c33c977
Update governance/proposals/010-arbitrum-l1-l2-messaging.js
clemsos Apr 3, 2024
79327c5
use ethers5
clemsos Apr 3, 2024
ace2d13
refactor proposal
clemsos Apr 3, 2024
794914d
Merge branch 'master' of github.com:unlock-protocol/unlock into l1-l2…
clemsos Apr 3, 2024
a82a7c0
* Revert change to .gitignore, and .env.copy
blahkheart Apr 3, 2024
debbbdf
* Update proposal description
blahkheart Apr 3, 2024
6687533
Resolved yarn.lock conflict
blahkheart Apr 4, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion .clabot
clemsos marked this conversation as resolved.
Show resolved Hide resolved
Expand Up @@ -62,7 +62,8 @@
"0xTxbi",
"FedericoCaruso",
"njokuScript",
"Calla-Ji"
"Calla-Ji",
"blahkheart"
],
"message": "Thank you for your pull request and welcome to Unlock! We require contributors to sign our [Contributor License Agreement](https://github.com/unlock-protocol/unlock/blob/master/CLA.txt), and we don't seem to have the users {{usersWithoutCLA}} on file. \nIn 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`.\nThank you! "
}
27 changes: 27 additions & 0 deletions governance/.env
clemsos marked this conversation as resolved.
Show resolved Hide resolved
@@ -0,0 +1,27 @@
# private key of the signer that will deploy the contract
export DEPLOYER_PRIVATE_KEY="0x14e1a3103ce06d0348acf19df3df1965ee55f9cdb16708c0bb8cd3d843ab8805"

# block explorers API keys
export ETHERSCAN_API_KEY="988a82bfe0a540ad84aa1a5d534c2bc6"
export POLYGONSCAN_API_KEY=
export OPTIMISTIC_ETHERSCAN_API_KEY=
export BSCSCAN_API_KEY=
export SNOWTRACE_API_KEY=
export ARBISCAN_API_KEY=
export CELO_API_KEY=
export GNOSISSCAN_API_KEY=

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

# Duplicate this file as .env

# Your Private key
DEVNET_PRIVKEY="0x14e1a3103ce06d0348acf19df3df1965ee55f9cdb16708c0bb8cd3d843ab8805"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


# Hosted Aggregator Node (JSON-RPC Endpoint). This is Arbitrum Sepolia Testnet, can use any Arbitrum chain

L2RPC="https://arbitrum-mainnet.infura.io/v3/988a82bfe0a540ad84aa1a5d534c2bc6"
clemsos marked this conversation as resolved.
Show resolved Hide resolved

# Ethereum RPC; i.e., for Sepolia https://sepolia.infura.io/v3/<your infura key>

L1RPC="https://mainnet.infura.io/v3/988a82bfe0a540ad84aa1a5d534c2bc6"
17 changes: 16 additions & 1 deletion governance/.env.copy
Expand Up @@ -9,4 +9,19 @@ export BSCSCAN_API_KEY=
export SNOWTRACE_API_KEY=
export ARBISCAN_API_KEY=
export CELO_API_KEY=
export GNOSISSCAN_API_KEY=
export GNOSISSCAN_API_KEY=
Copy link
Member

Choose a reason for hiding this comment

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

Why is this in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't add that, it was already in the .env.copy file

Copy link
Member

Choose a reason for hiding this comment

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

Why is it in the diff of this pull-request then?

Copy link
Member

Choose a reason for hiding this comment

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

its a deleted newline it seems

Copy link
Member

Choose a reason for hiding this comment

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

yeah that change should not be in this PR.


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

Choose a reason for hiding this comment

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

Please remeove these changes...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay got it


# Duplicate this file as .env

# Your Private key
DEVNET_PRIVKEY="0x your key here"

# Hosted Aggregator Node (JSON-RPC Endpoint). This is Arbitrum Sepolia Testnet, can use any Arbitrum chain

L2RPC="https://sepolia-rollup.arbitrum.io/rpc"

# Ethereum RPC; i.e., for Sepolia https://sepolia.infura.io/v3/<your infura key>

L1RPC=""
1 change: 1 addition & 0 deletions governance/.gitignore
Expand Up @@ -2,6 +2,7 @@
cache
contracts
artifacts
.env
Copy link
Member

Choose a reason for hiding this comment

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

same here


# zksync
zk-artifacts
Expand Down
5 changes: 4 additions & 1 deletion governance/package.json
Expand Up @@ -3,6 +3,8 @@
"description": "Scripts for the management of the Unlock Protocol",
"private": true,
"dependencies": {
"@arbitrum/nitro-contracts": "v1.0.2",
"@arbitrum/sdk": "^v3.1.9",
Copy link
Member

Choose a reason for hiding this comment

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

Please use fixed version numbers (no v or ^)

"@matterlabs/hardhat-zksync-deploy": "1.1.2",
"@matterlabs/hardhat-zksync-solc": "1.1.0",
"@matterlabs/hardhat-zksync-upgradable": "1.2.4",
Expand All @@ -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",
Copy link
Member

Choose a reason for hiding this comment

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

Same here!

"eslint": "8.54.0",
"ethers": "6.10.0",
"ethers": "6.11.1",
Copy link
Member

Choose a reason for hiding this comment

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

Is that upgrade needed for the PR to work ?

If not could you remove the change in version (better to have it in a separate PR as it may break other scripts on the repo).

"fs-extra": "11.2.0",
"hardhat": "2.20.1",
"solhint": "4.1.1",
Expand Down
263 changes: 263 additions & 0 deletions governance/proposals/010-arbitrum-l1-l2-messaging.js
@@ -0,0 +1,263 @@
const ethers = require('ethers')
const {
L1ToL2MessageGasEstimator,
} = require('@arbitrum/sdk/dist/lib/message/L1ToL2MessageGasEstimator')
const { arbLog } = require('arb-shared-dependencies')
const {
EthBridger,
getL2Network,
addDefaultLocalNetwork,
} = require('@arbitrum/sdk')
const { getBaseFee } = require('@arbitrum/sdk/dist/lib/utils/lib')

const ERC20_ABI = [
{
inputs: [
{
internalType: 'address',
name: 'account',
type: 'address',
},
],
name: 'balanceOf',
outputs: [
{
internalType: 'uint256',
name: '',
type: 'uint256',
},
],
stateMutability: 'view',
type: 'function',
},
{
inputs: [
{
internalType: 'address',
name: 'to',
type: 'address',
},
{
internalType: 'uint256',
name: 'amount',
type: 'uint256',
},
],
name: 'transfer',
outputs: [
{
internalType: 'bool',
name: '',
type: 'bool',
},
],
stateMutability: 'nonpayable',
type: 'function',
},
]
clemsos marked this conversation as resolved.
Show resolved Hide resolved
const INBOX_ABI = [
{
inputs: [
{
internalType: 'address',
name: 'to',
type: 'address',
},
{
internalType: 'uint256',
name: 'l2CallValue',
type: 'uint256',
},
{
internalType: 'uint256',
name: 'maxSubmissionCost',
type: 'uint256',
},
{
internalType: 'address',
name: 'excessFeeRefundAddress',
type: 'address',
},
{
internalType: 'address',
name: 'callValueRefundAddress',
type: 'address',
},
{
internalType: 'uint256',
name: 'gasLimit',
type: 'uint256',
},
{
internalType: 'uint256',
name: 'maxFeePerGas',
type: 'uint256',
},
{
internalType: 'bytes',
name: 'data',
type: 'bytes',
},
],
name: 'createRetryableTicket',
outputs: [
{
internalType: 'uint256',
name: '',
type: 'uint256',
},
],
stateMutability: 'payable',
type: 'function',
},
]

/**
* Set up: instantiate L1 / L2 wallets connected to providers
*/
const walletPrivateKey = process.env.DEVNET_PRIVKEY
const l1Provider = new ethers.JsonRpcProvider(process.env.L1RPC)
const l2Provider = new ethers.JsonRpcProvider(process.env.L2RPC)
// const l1Wallet = new ethers.Wallet(walletPrivateKey, l1Provider)
const l2Wallet = new ethers.Wallet(walletPrivateKey, l2Provider)

module.exports = async () => {
await arbLog('Cross-chain Proposer')
Copy link
Member

Choose a reason for hiding this comment

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

what is arbLog used for ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Can we just use a regular console.log please?

addDefaultLocalNetwork()

const l2Network = await getL2Network(l2Provider)
const ethBridger = new EthBridger(l2Network)
const inboxAddress = ethBridger.l2Network.ethBridge.inbox
const ARBTokenAddressOnL2 = '0x912CE59144191C1204E64559FE8253a0e49E6548' // ARB TOKEN ADDRESS ON ARBITRUM ONE
Copy link
Member

Choose a reason for hiding this comment

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

Can yoiu please use the address (or add it there) in the package/networks file?

const grantsContractAddress = '0x00D5E0d31d37cc13C645D86410aB4cB7Cb428ccA'
const L2Alias = '0x28ffDfB0A6e6E06E95B3A1f928Dc4024240bD87c' // Timelock Alias Address on L2
const L1TimelockContract = '0x17EEDFb0a6E6e06E95B3A1F928dc4024240BC76B' // Timelock Address mainnet
Copy link
Member

Choose a reason for hiding this comment

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

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


const L2TokenContract = new ethers.Contract(
ARBTokenAddressOnL2,
ERC20_ABI,
l2Wallet
).connect(l2Wallet)

const balanceOf = await L2TokenContract.balanceOf(L2Alias)
console.log('ARB BALANCE::', ethers.formatEther(await balanceOf))

const tokenAmount = ethers.parseEther('1')
Copy link
Member

Choose a reason for hiding this comment

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

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.


// Create an instance of the Interface from the ABIs
const iface_erc20 = new ethers.Interface(ERC20_ABI)
Copy link
Member

Choose a reason for hiding this comment

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

Please respect the styleguide if you can:

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

const iface_inbox = new ethers.Interface(INBOX_ABI)

// Encode the ERC20 Token transfer calldata
const transfer_calldata = iface_erc20.encodeFunctionData('transfer', [
Copy link
Member

Choose a reason for hiding this comment

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

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

grantsContractAddress,
tokenAmount,
])
/**
* Now we can query the required gas params using the estimateAll method in Arbitrum SDK
*/
const l1ToL2MessageGasEstimate = new L1ToL2MessageGasEstimator(l2Provider)

/**
* The estimateAll method gives us the following values for sending an L1->L2 message
* (1) maxSubmissionCost: The maximum cost to be paid for submitting the transaction
* (2) gasLimit: The L2 gas limit
* (3) deposit: The total amount to deposit on L1 to cover L2 gas and L2 call value
*/
const L1ToL2MessageGasParams = await l1ToL2MessageGasEstimate.estimateAll(
{
from: L1TimelockContract,
to: ARBTokenAddressOnL2,
l2CallValue: 0,
excessFeeRefundAddress: l2Wallet.address,
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why l2Wallet would recive the excess here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

callValueRefundAddress: l2Wallet.address,
Copy link
Member

Choose a reason for hiding this comment

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

Same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reason as before

data: transfer_calldata,
},
await getBaseFee(l1Provider),
l1Provider
)
console.log(':::::::::L1ToL2MessageGasParams::::::::::')
console.log(
'GasParams::::gasLimit',
L1ToL2MessageGasParams.gasLimit.toNumber()
)
console.log(
'GasParams::::maxSubmissionCost',
L1ToL2MessageGasParams.maxSubmissionCost.toNumber()
)
console.log(
clemsos marked this conversation as resolved.
Show resolved Hide resolved
'GasParams::::maxGas',
L1ToL2MessageGasParams.maxFeePerGas.toNumber()
)
console.log('GasParams::::deposit', L1ToL2MessageGasParams.deposit.toNumber())

console.log(
`Current retryable base submission price is: ${L1ToL2MessageGasParams.maxSubmissionCost.toString()}`
)

const gasPriceBid = await l2Provider.getGasPrice()
console.log(`L2 gas price: ${gasPriceBid.toString()}`)

const inbox_calldata = iface_inbox.encodeFunctionData(
'createRetryableTicket',
[
ARBTokenAddressOnL2,
Copy link
Member

Choose a reason for hiding this comment

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

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

0,
L1ToL2MessageGasParams.maxSubmissionCost,
l2Wallet.address,
l2Wallet.address,
L1ToL2MessageGasParams.gasLimit,
gasPriceBid,
transfer_calldata,
]
)

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.`
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

console.log(proposalName)
// Proposal ARGS i.e Call Governor.propose() directly with these values
// targets: Inbox contract
// values: depost * 10
// calldatas: inbox_calldata
// description
const targets = [inboxAddress]
const values = [L1ToL2MessageGasParams.deposit.toNumber() * 10] // I Multiply by 10 to add extra in case gas changes
const calldatas = [inbox_calldata]
console.log(
'______________________________________________________________________\n'
)
console.log(
'PROPOSAL ARGS - Can Call Propose function on Governor with the following::'
)
console.log(
'______________________________________________________________________\n'
)

console.log('TARGETS:: ', targets)
console.log('VALUES:: ', values)
console.log('CALLDATAS:: ', calldatas)
console.log('DESCRIPTION:: ', proposalName)

const calls = [
{
contractNameOrAbi: INBOX_ABI,
contractAddress: inboxAddress,
functionName: 'createRetryableTicket',
functionArgs: [
ARBTokenAddressOnL2,
0,
L1ToL2MessageGasParams.maxSubmissionCost,
l2Wallet.address,
l2Wallet.address,
L1ToL2MessageGasParams.gasLimit,
gasPriceBid,
transfer_calldata,
],
value: L1ToL2MessageGasParams.deposit.toNumber() * 10, // I Multiply by 10 to add extra in case gas changes due to proposal delay
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

},
]

return {
proposalName,
calls,
}
}