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

Feature/setup dac #4

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Feature/setup dac #4

wants to merge 17 commits into from

Conversation

arnaubennassar
Copy link
Collaborator

@arnaubennassar arnaubennassar commented Apr 2, 2024

  • Deploy DAC and setup DAC commands
  • Added ABI and bin to generate Go code for OppenZeppelin admin proxy to be used to instantiate the DAC
  • Added commands to start / stop / export a Dockerized Mock L1 based on geth in dev mode

Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

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

Generally LGTM.

Comment on lines +43 to +45
func getClient() (*ethclient.Client, error) {
return ethclient.Dial("http://localhost:8545")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably doesn't deserve a separate function for this. WDYT?

Also as a side note, would it be an overkill if we were starting up the mock l1 network in case it is not running already?

if err != nil {
return err
}
os.Geteuid()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this line a leftover?

Suggested change
os.Geteuid()

return errors.New("failed to fund account")
}

return err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional nitpick (it is probably just slightly better, in terms of readability being explicit here, but as you prefer at the end of the day)

Suggested change
return err
return nil

@@ -0,0 +1,18 @@
#!/bin/sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

WDYT about reusing this script here as well

err = exec.Command(
"bash", "-l", "-c",
fmt.Sprintf(
"abigen --bin bin/%s.bin --abi abi/%s.abi --pkg=%s --out=%s/%s.go",
name, name, goName, goName, goName,
),
).Run()
?

&cli.PathFlag{
Name: setupDACFilePathFlagName,
Aliases: []string{"f"},
Usage: `File path of a JSON that looks like {"requiredSingatures": X, "members": [{"address": "0x...", "url": "http://..."}]}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Usage: `File path of a JSON that looks like {"requiredSingatures": X, "members": [{"address": "0x...", "url": "http://..."}]}`,
Usage: `File path of a JSON that looks like {"requiredSignatures": X, "members": [{"address": "0x...", "url": "http://..."}]}`,

Probably it would be a good idea providing a JSON example as it is done for the wallets and rpcs toml files?

}
fmt.Printf(`
DAC Configuration:
required signatures %d / %d.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
required signatures %d / %d.
Required signatures %d / %d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants