-
Notifications
You must be signed in to change notification settings - Fork 75
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
feat: support for adding interop contracts to forked networks #87
feat: support for adding interop contracts to forked networks #87
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @tremarkley and the rest of your teammates on Graphite |
e37e0e6
to
a623ea3
Compare
a623ea3
to
291d114
Compare
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.
looks good! some minor suggestions
we should add tests to sanity check that the contracts work, but can be in separate PR
opsimulator/opsimulator.go
Outdated
opSim.startStartupTasks() | ||
if err := opSim.startupTasks.Wait(); err != nil { | ||
return fmt.Errorf("failed to start opsimulator: %w", err) |
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.
nit: wdyt about putting the for loop inside a single startupTask? The main point of the startupTasks field was to better separate tasks that are run before the opsim returns ready vs. ones that can run after opsim returns ready.
That way in a separate entrypoint, a caller can just wait on startupTasks to see if opsim is ready
opSim.startupTasks.Go(func() error {
for chain in ...
addDependencySet
}
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.
sounds good, I updated the startupTasks to include configureInteropForChain
and addDependencies
genesis/applier.go
Outdated
namespaceBigInt := new(big.Int).SetBytes(common.FromHex("0xc0D3C0d3C0d3C0D3c0d3C0d3c0D3C0d3c0d30000")) | ||
resultBigInt := new(big.Int).Or(new(big.Int).And(addrBigInt, big.NewInt(0xffff)), namespaceBigInt) |
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.
nit: might make sense to just hard code the impl addresses as well. since there's only 3, and this is temporary code
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 think this function is useful if we ever need to add more contracts or if the proxy addresses were ever changed for some reason. Is there a reason you prefer to hardcode the values?
genesis/applier.go
Outdated
|
||
func fetchAllocForAddr(addr common.Address, allocsJSON []byte) (*alloc, error) { | ||
var allocs allocs | ||
err := json.Unmarshal(allocsJSON, &allocs) |
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 can prob memoize the result of the unmarshal and store in memory so we don't have to unmarshal everytime. esp since this is called at the startup we should try to reduce latency as much possible
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.
good call, memoized it at the top level call opsimulator.configureInteropForChain
9f27a5b
to
a54af04
Compare
a54af04
to
7377a66
Compare
Closes #69