-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Foundry breaks chainId on optimism mainnet deployment #4362
Comments
ptal @joshieDo |
@mattsse do you have any idea what could be the issue here? Was first thinking might be related to chainId de/serialization, but as it's not always wrong seems unlikely. Sadly, there's bit of time pressure here so will probably port back deploy scripts to hardhat for the time being. That said, would be super interested to figure out what the issue was/so in the future we can use foundry also with optimism 😅 |
Try sending legacy transactions with the |
@tynes tried, but no difference |
nothing stuck out to me as well, can you please run with |
Log of a failing txn showing as
Log of a txn showing as
I don't really see any difference, both shows |
okay this looks like an rlp issue for legacy tx, giga cursed. checking rn |
oh I think I've found it |
oh nevermind, I did not. I does not make any sense why ledger would see the chain id as |
this is the relevant ledger code. doesn't make sense if chain_id is already set to any ideas @prestwich ? |
🤔 this behavior would make sense if neither the Where is the Signer chain_id set in the forge setup logic? |
this is the relevant section foundry/cli/src/cmd/forge/script/broadcast.rs Lines 572 to 599 in 9e47508
what's weird is, the trace posted above clearly contains the chain id before when hand this over to ledger |
I first thought there's an issue with and the rlp function used during signing does set chain id |
if the else branch in the function is hit, it'd produce this I believe |
@sakulstra when you run with foundry/cli/src/opts/wallet/multi_wallet.rs Line 446 in 9e47508
|
Have reproduced. Setting
this RLP string is exactly 3 bytes shorter than you would expect for a valid RLP-serialized tx. The three bytes that are missing would correspond to chain_id, r, and s. This could explain why chain_id is 0. If Ledger has permissive deser for these fields (code is here, but i haven't analyzed it at all) then they might be 0 after deser of the invalid RLP have to leave my desk, but will dig deeper re why the RLP is being generated invalid in the first place when i get back |
looks like this is a bug in the app on the ledger device. Optimism was a red herring |
Just tested, and have confirmed that gakonst/ethers-rs#2192 will mitigate. Device now shows Optimism :) |
should be closed by #4477 |
@mattsse is this released? Tried to verify, but for me on latest nightly the issue still persists. |
not in nightly yet, triggering release now, should be available in ~45min |
Possible this issue was somehow reintroduced? Will try to provide a reproduction next week 😅 |
there haven't been any changes to these codepaths in the ledger signer, so it seems unlikely the issue was reintroduced this is, ofc, an issue with the ledger ethereum app, not with ethers-rs, so no amount of mitigation we do can fix it 😮💨 |
Component
Forge
Have you ensured that all of these are up to date?
What version of Foundry are you on?
forge 0.2.0 (550c548 2023-02-13T00:11:18.976721967Z)
What command(s) is the bug in?
forge script
Operating System
Linux
Describe the bug
For certain transactions on optimism network - i can't determine what is the condition, but managed to create a reliable reproduction - foundry sends
chainId 0
.Things I tried:
Reproduction:
make recover-optimism-ledger
with a connected ledger.network 0
References:
ux issues
while this one seems to be a real blocker for using foundry with optimism.The text was updated successfully, but these errors were encountered: