Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Arbitrum L1 to L2 messaging proposal #13516
Changes from 4 commits
f0cbaf2
85376b1
247edf0
2f0608f
b9c3467
20939ee
506fdc6
09aada2
6cd79c2
cd41637
80b6fe6
14273ca
6709e11
a6942e4
b54ffa4
347d6bf
98935ee
e98703b
6c9f33a
3e817b7
39b4e79
24d81c3
eabfe4d
cbdfb95
fa63c3d
0f7ec36
ce0e970
c1fd7d7
7bced2f
0a993b8
c9a28c1
4d42172
c33c977
79327c5
ace2d13
794914d
a82a7c0
debbbdf
6687533
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remeove these changes...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay got it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
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^
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here!
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 ?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 theexecute()
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason as before
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.