Persistence Updates #56
Conversation
…rs, updating docker compose to pass local environment variables into docker
There was a problem hiding this comment.
This looks good so far but I haven't quite finished the review. Instead I just noticed a bit of terminology that I got confused by and so I'm submitting this as a comment to then send around to make sure it lines up with others intuitions.
Might be good to add stuff like this to a glossary--we've got so many moving parts it's important we're all on the same page with what is what.
| * to consolidate access / updates and default values. | ||
| */ | ||
| export class Environment { | ||
| // Local Node Config |
There was a problem hiding this comment.
Looks good! In the spirit of honing my code style intuitions, I have two observations:
- These comments are good because they provide context for the organizational structure of the code, not just what the code does.
- We could possibly look for a library which handles config but in this case building this class seems like it wouldn't be much harder than using a library, so it's fine to use a "custom" implementation.
| * | ||
| * @param mnemonic | ||
| * @param port | ||
| */ | ||
| export const startLocalL1Node = async ( |
There was a problem hiding this comment.
Hmm the terminology here is a little confusing. Am I getting it right that "LocalL1Node" is what we send "InternalTransactions" to? It's just really confusing and I want to make sure we have all of the components mapped out with reasonable terminology.
I just made a diagram which illustrates all of the components.
So to be clear, L1 Node would be just a normal Geth instance that's syncing mainnet Ethereum. Then an L2 Node or maybe more specifically Rollup Node would be the thing that runs an Internal instance of Geth / EVM, which itself either emulates an OVM deployed to it (ExecutionManger, etc).
Does that match up with the terminology being used here?
There was a problem hiding this comment.
Yep, that's it! The line in question is to specifically start an L1 node locally to fully exercise the end-to-end functionality of L2 <--> L1 messaging. This is completely different than the L2 Node. I went with L1 and L2 because nothing is actually "rolled up" in the L2 node, and instead, the rollup state sits in the L1 node. Wanted to avoid that confusion. Does L1 and L2 sound good? Happy to rename it to rollup if you think it makes more sense.
There was a problem hiding this comment.
Ah great! So I think after staring at this for a while I see where my confusion was. The fact that inside the LocalL1Node we have a sequencerWallet was throwing me off. But it’s just because in this circumstance the L1 that we are running is a Local testnet run by the sequencer.
These L1 & L2 terms are a little confusing! Especially when you see L1ToL2 right next to L2ToL1 — my dyslexia goes crazy! That said, it’s not clear to me that EthNode and RollupNode (for example) are much better 😕 Especially because both are in some sense “Eth” nodes.
I realize this could require some refactoring/renaming (and so I’m happy to not do it) but I wonder if instead of L1ToL2/L2ToL1, we could use the terms L2InboundMessages and L2OutboundMessages or something? Just spitballing and curious what you / @ben-chain think.
It’s a nitpick though & might be specific to my difficulty reading 😂
There was a problem hiding this comment.
Oh nvm I take it back. I’m now thinking about l2ToL1MessageReceiver and calling it l2OutboundMessageReceiver and realize that these variable names are going to be confusing basically no matter how we slice it.
Unless y'all absolutely love my naming change (which might have been closer to what Ben did originally?) doesn’t feel worth the switching cost.
…t use geth until we determine private keys / mnemonics and configs
| * | ||
| * @param mnemonic | ||
| * @param port | ||
| */ | ||
| export const startLocalL1Node = async ( |
There was a problem hiding this comment.
Ah great! So I think after staring at this for a while I see where my confusion was. The fact that inside the LocalL1Node we have a sequencerWallet was throwing me off. But it’s just because in this circumstance the L1 that we are running is a Local testnet run by the sequencer.
These L1 & L2 terms are a little confusing! Especially when you see L1ToL2 right next to L2ToL1 — my dyslexia goes crazy! That said, it’s not clear to me that EthNode and RollupNode (for example) are much better 😕 Especially because both are in some sense “Eth” nodes.
I realize this could require some refactoring/renaming (and so I’m happy to not do it) but I wonder if instead of L1ToL2/L2ToL1, we could use the terms L2InboundMessages and L2OutboundMessages or something? Just spitballing and curious what you / @ben-chain think.
It’s a nitpick though & might be specific to my difficulty reading 😂
| * | ||
| * @param mnemonic | ||
| * @param port | ||
| */ | ||
| export const startLocalL1Node = async ( |
There was a problem hiding this comment.
Oh nvm I take it back. I’m now thinking about l2ToL1MessageReceiver and calling it l2OutboundMessageReceiver and realize that these variable names are going to be confusing basically no matter how we slice it.
Unless y'all absolutely love my naming change (which might have been closer to what Ben did originally?) doesn’t feel worth the switching cost.
* decompression (not working) * start EIP155 serialization * working decompressor * working CreateEOA, working native EIP155tx * all three tx types working * remove only * fix ECDSAUtil tests * restore original version of smock * add proxys * clean up tests * clean up decompressor tests * clean up ProxyDecompressor * clean up decompressor tests * add CREATEEOA, SETNONCE, GETNONCE tests * working ECDSAContractAccount * fix decompressor * renaming, add deploy * add proxyeoa test, cleanup * remove createEOA type, add extcodesize check * fix decompressor test * support create txs * Decompressor -> Entrypoint * use safeREVERT instead of requires * add isAuthenticated state manager test * update smock * fix sequencerEntrypoint test * add isAuthenticated * Revert "update smock" This reverts commit 286a222414accf9387dcd12ecbed0a66be586bc8. * fix test * Updates to EM * removed kall from contract account * Fixed SSLOAD and SSTORE problems * Fix some broken tests * Fix more bugs & rename to ProxyEOA * WIP fix call test * fix all tests * add gas check * update tests * lint and remove 2000 gas check * Add fees to default EOA (#56) Co-authored-by: Kelvin Fichter <[email protected]> Co-authored-by: Karl Floersch <[email protected]>
…ting-exemptions add more linting exemptions
…10.0 (ethereum-optimism#56) * deps: bump celestia-node v0.9.5 to v0.10.0 * change RPC * swap RPC * deps: bump v0.9.5 to v0.10.0 local-celestia-devnet
Description
Adding and cleaning up configuration via environment variables. Trying to pave the way for persistence in a deployed dev environment.
Contributing Agreement