Change config to use RPC urls instead of host and port #1512

BedrockSquirrel · 2023-09-12T11:23:41Z

Why this change is needed

For sepolia we want to pass in URLs for API services like infura but our config all expects host+port separately and constructs the URL.

What changes were made as part of this PR

Change host, contract deployer configs and github deploy actions to use RPC addresses instead of host + port.

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required
PR checks

PR checks reviewed and performed

otherview

Looks good, I would suggest a slight change to the naming though.
Namely changing all names to variations of L1 Websocket URL instead of L1 websocket RPC or Address. I believe URL is the name we're looking for as it describes protocol + address + port.

otherview · 2023-09-12T13:04:09Z

.github/workflows/manual-deploy-testnet-l2.yml

+      L1_WS_RPC: ${{ steps.outputVars.outputs.L1_WS_RPC }}
+      L1_HTTP_RPC: ${{ steps.outputVars.outputs.L1_HTTP_RPC }}


Sugestion:

Suggested change

L1_WS_RPC: ${{ steps.outputVars.outputs.L1_WS_RPC }}

L1_HTTP_RPC: ${{ steps.outputVars.outputs.L1_HTTP_RPC }}

L1_WS_URL: ${{ steps.outputVars.outputs.L1_WS_RPC }}

L1_HTTP_URL: ${{ steps.outputVars.outputs.L1_HTTP_RPC }}

Typically this is called URL because it's proto + address + port formatted

otherview · 2023-09-12T13:09:42Z

.github/workflows/manual-deploy-testnet-l2.yml

@@ -107,7 +111,7 @@ jobs:
        shell: bash
        run: |
          go run ./testnet/launcher/l1contractdeployer/cmd \
-          -l1_host=${{ env.L1_HOST }} \
+          -l1_http_rpc=${{ env.L1_HTTP_RPC }} \


Suggested change

-l1_http_rpc=${{ env.L1_HTTP_RPC }} \

-l1_http_url=${{ env.L1_HTTP_RPC }} \

otherview · 2023-09-12T13:10:35Z

.github/workflows/manual-upgrade-testnet-l2.yml

@@ -62,8 +62,8 @@ jobs:
          echo "RESOURCE_TAG_NAME=testnetlatest" >> $GITHUB_ENV
          echo "RESOURCE_STARTING_NAME=T" >> $GITHUB_ENV
          echo "RESOURCE_TESTNET_NAME=testnet" >> $GITHUB_ENV
-          echo "L1_HOST=testnet-eth2network.uksouth.cloudapp.azure.com" >> $GITHUB_ENV
-
+          echo "L1_WS_RPC=ws://testnet-eth2network.uksouth.cloudapp.azure.com:9000" >> $GITHUB_ENV


Suggested change

echo "L1_WS_RPC=ws://testnet-eth2network.uksouth.cloudapp.azure.com:9000" >> $GITHUB_ENV

echo "L1_WS_URL=ws://testnet-eth2network.uksouth.cloudapp.azure.com:9000" >> $GITHUB_ENV

otherview · 2023-09-12T13:11:26Z

go/config/host_config.go

+	// Websocket RPC address for interactions with the L1
+	L1WebsocketRPCAddress string


Suggested change

// Websocket RPC address for interactions with the L1

L1WebsocketRPCAddress string

// L1WebsocketURL is the RPC address for interactions with the L1

L1WebsocketURL string

Keeping the same name for consistency

otherview · 2023-09-12T13:11:54Z

go/config/host_config.go

+	// The websocket RPC address for L1 requests
+	L1WebsocketRPCAddr string


Suggested change

// The websocket RPC address for L1 requests

L1WebsocketRPCAddr string

// L1WebsocketURL is the RPC address for interactions with the L1

L1WebsocketURL string

otherview · 2023-09-12T13:12:08Z

go/host/container/cli.go

@@ -27,8 +27,7 @@ type HostConfigToml struct {
 	EnclaveRPCAddress         string
 	P2PBindAddress            string
 	P2PPublicAddress          string
-	L1NodeHost                string
-	L1NodeWebsocketPort       uint
+	L1WebsocketRPCAddress     string


Suggested change

L1WebsocketRPCAddress string

L1WebsocketURL string

otherview · 2023-09-12T13:12:42Z

go/host/container/cli_flags.go

@@ -12,8 +12,7 @@ const (
 	enclaveRPCAddressName        = "enclaveRPCAddress"
 	p2pBindAddressName           = "p2pBindAddress"
 	p2pPublicAddressName         = "p2pPublicAddress"
-	l1NodeHostName               = "l1NodeHost"
-	l1NodePortName               = "l1NodePort"
+	l1WebsocketRPCAddrName       = "l1WSRPCAddress"


Suggested change

l1WebsocketRPCAddrName = "l1WSRPCAddress"

l1WebsocketURLName = "l1WSURL"

otherview · 2023-09-12T13:14:14Z

go/node/cmd/cli_flags.go

@@ -41,8 +40,7 @@ func getFlagUsageMap() map[string]string {
 		isSGXEnabledFlag:            "Whether the it should run on an SGX is enabled CPU",
 		enclaveDockerImageFlag:      "Docker image for the enclave",
 		hostDockerImageFlag:         "Docker image for the host",
-		l1HostFlag:                  "Layer 1 network host addr",
-		l1WSPortFlag:                "Layer 1 network WebSocket port",
+		l1WSRPCAddressFlag:          "Layer 1 websocket RPC address",


Suggested change

l1WSRPCAddressFlag: "Layer 1 websocket RPC address",

l1WSURLFlag: "Layer 1 WebSocket RPC URL",

otherview · 2023-09-12T16:32:31Z

go/host/container/host_container.go

@@ -114,7 +114,7 @@ func NewHostContainerFromConfig(parsedConfig *config.HostInputConfig, logger get
 	ethWallet := wallet.NewInMemoryWalletFromConfig(cfg.PrivateKeyString, cfg.L1ChainID, log.New("wallet", cfg.LogLevel, cfg.LogPath))

 	fmt.Println("Connecting to L1 network...")
-	l1Client, err := ethadapter.NewEthClient(cfg.L1NodeHost, cfg.L1NodeWebsocketPort, cfg.L1RPCTimeout, cfg.ID, logger)
+	l1Client, err := ethadapter.NewEthClientFromAddress(cfg.L1WebsocketURL, cfg.L1RPCTimeout, cfg.ID, logger)


Suggested change

l1Client, err := ethadapter.NewEthClientFromAddress(cfg.L1WebsocketURL, cfg.L1RPCTimeout, cfg.ID, logger)

l1Client, err := ethadapter.NewEthClientFromURL(cfg.L1WebsocketURL, cfg.L1RPCTimeout, cfg.ID, logger)

If you have the patience it would cool if we changed NewEthClient to call NewEthClientFromAddress instead the two methods having the same duplicated code.

otherview · 2023-09-12T16:36:04Z

testnet/launcher/l1contractdeployer/cmd/cli_flags.go

@@ -13,8 +12,7 @@ const (
 // While we could just use constants instead of a map, this approach allows us to test that all the expected flags are defined.
 func getFlagUsageMap() map[string]string {
 	return map[string]string{
-		l1HostFlag:           "Layer 1 network host addr",
-		l1HTTPPortFlag:       "Layer 1 network HTTP port",
+		l1HTTPRPCAddressFlag: "Layer 1 network http RPC addr",


Suggested change

l1HTTPRPCAddressFlag: "Layer 1 network http RPC addr",

l1HTTPURLFlag: "Layer 1 network http RPC addr",

otherview · 2023-09-12T16:36:27Z

testnet/launcher/l1contractdeployer/cmd/main.go

-			l1cd.WithL1Port(cliConfig.l1HTTPPort),       // 8025
-			l1cd.WithPrivateKey(cliConfig.privateKey),   //"f52e5418e349dccdda29b6ac8b0abe6576bb7713886aa85abea6181ba731f9bb"),
-			l1cd.WithDockerImage(cliConfig.dockerImage), //"testnetobscuronet.azurecr.io/obscuronet/hardhatdeployer:latest"
+			l1cd.WithL1HTTPRPCAddress(cliConfig.l1HTTPRPCAddr), // "http://eth2network:8025"


Suggested change

l1cd.WithL1HTTPRPCAddress(cliConfig.l1HTTPRPCAddr), // "http://eth2network:8025"

l1cd.WithL1HTTPURL(cliConfig.l1HTTPRPCAddr), // "http://eth2network:8025"

otherview · 2023-09-12T16:36:44Z

testnet/launcher/l1contractdeployer/config.go

-	privateKey  string
-	l1Port      int
-	dockerImage string
+	l1HTTPRPCAddress string


Suggested change

l1HTTPRPCAddress string

l1HTTPURL string

otherview · 2023-09-12T16:37:00Z

testnet/launcher/l2contractdeployer/cmd/cli.go

@@ -6,8 +6,7 @@ import (

 // L2ContractDeployerConfigCLI represents the configurations passed into the deployer over CLI
 type L2ContractDeployerConfigCLI struct {
-	l1Host                 string
-	l1HTTPPort             int
+	l1HTTPRPCAddr          string


Suggested change

l1HTTPRPCAddr string

l1HTTPURL string

otherview

lgtm - would optionally make that NewEthClientFromAddress change (name and reusage), but it's totally fine to leave it to another pr too.

The contract deployers should probably get updated, would suggest taking those sugestion-changes, so that we call the l1 http url everywhere the same.

Change config to use RPC urls instead of host and port

d9f9038

BedrockSquirrel requested a review from otherview September 12, 2023 11:27

BedrockSquirrel added 2 commits September 12, 2023 12:37

fix toml test

f806406

Merge remote-tracking branch 'origin/main' into matt/rpc-address-config

456ccc9

otherview reviewed Sep 12, 2023

View reviewed changes

BedrockSquirrel added 2 commits September 12, 2023 14:58

renames

e8401a0

missed some

a9a46cf

otherview reviewed Sep 12, 2023

View reviewed changes

otherview approved these changes Sep 12, 2023

View reviewed changes

review comments

e72096f

BedrockSquirrel merged commit b7b4e3e into main Sep 13, 2023

BedrockSquirrel deleted the matt/rpc-address-config branch September 13, 2023 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change config to use RPC urls instead of host and port #1512

Change config to use RPC urls instead of host and port #1512

BedrockSquirrel commented Sep 12, 2023

otherview left a comment

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview Sep 12, 2023

otherview left a comment

		L1_WS_RPC: ${{ steps.outputVars.outputs.L1_WS_RPC }}
		L1_HTTP_RPC: ${{ steps.outputVars.outputs.L1_HTTP_RPC }}

	-l1_http_rpc=${{ env.L1_HTTP_RPC }} \
	-l1_http_url=${{ env.L1_HTTP_RPC }} \

	echo "L1_WS_RPC=ws://testnet-eth2network.uksouth.cloudapp.azure.com:9000" >> $GITHUB_ENV
	echo "L1_WS_URL=ws://testnet-eth2network.uksouth.cloudapp.azure.com:9000" >> $GITHUB_ENV

		// Websocket RPC address for interactions with the L1
		L1WebsocketRPCAddress string

		// The websocket RPC address for L1 requests
		L1WebsocketRPCAddr string

	l1WebsocketRPCAddrName = "l1WSRPCAddress"
	l1WebsocketURLName = "l1WSURL"

	l1WSRPCAddressFlag: "Layer 1 websocket RPC address",
	l1WSURLFlag: "Layer 1 WebSocket RPC URL",

	l1Client, err := ethadapter.NewEthClientFromAddress(cfg.L1WebsocketURL, cfg.L1RPCTimeout, cfg.ID, logger)
	l1Client, err := ethadapter.NewEthClientFromURL(cfg.L1WebsocketURL, cfg.L1RPCTimeout, cfg.ID, logger)

	l1HTTPRPCAddressFlag: "Layer 1 network http RPC addr",
	l1HTTPURLFlag: "Layer 1 network http RPC addr",

	l1cd.WithL1HTTPRPCAddress(cliConfig.l1HTTPRPCAddr), // "http://eth2network:8025"
	l1cd.WithL1HTTPURL(cliConfig.l1HTTPRPCAddr), // "http://eth2network:8025"

Change config to use RPC urls instead of host and port #1512

Change config to use RPC urls instead of host and port #1512

Conversation

BedrockSquirrel commented Sep 12, 2023

Why this change is needed

What changes were made as part of this PR

PR checks pre-merging

otherview left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otherview left a comment

Choose a reason for hiding this comment