feat(admin): added getL1Addresses(chainId) rpc support #254

s4rv4d · 2024-11-08T18:11:09Z

Description

added getL1Addresses(chainId) support with /getL1Addresses/:chainID, essentially letting devs check L1 address for a particular L2 chain via chain ID

Additional context
Just prints L1 address for particular L2 chain via chain ID

Metadata

adds extra functionality requested in supersim admin API (read) #204

s4rv4d · 2024-11-08T18:17:53Z

sharing some ss for examples

jakim929 · 2024-11-09T00:57:25Z

@s4rv4d thank you for the contribution!

However, this PR currently feels a bit far off, partly my fault because the issue was underspecified.

It's not using JSON RPC - since this will be the basis for all the other admin read endpoints, it should be built in a reusable way.
It's not looking up the correct config for the chainId - it's currently looking it up by port
the chain config it returns should be based on the network config not the default config

Please let me know if you have any questions about the implementation.

s4rv4d · 2024-11-09T05:54:55Z

Got it working on this

s4rv4d · 2024-11-12T16:02:28Z

the chain config it returns should be based on the network config not the default config

the network config is getting init by GetDefaultNetworkConfig, if this isnt the right implementation, can you tell me which other way to init networkConfig?

s4rv4d · 2024-11-13T08:26:19Z

@jakim929 can you pls review this, just wanted to see if im approaching this the right way

jakim929 · 2024-11-13T16:49:23Z

admin/admin.go

+		defer s.wg.Done()
+
+		http.Handle("/", rpcServer)
+		err := http.ListenAndServe(fmt.Sprintf(":%d", s.port+1), nil)


the rpc server should just be on the same port as the specified

there should be very limited direct http endpoints here so

/: defaults to rpc
/ready: probably the only http endpoint we need

these can all be on the same port since they are separated by the route

awesome got it, making these revisions

jakim929 · 2024-11-13T16:54:09Z

the network config is getting init by GetDefaultNetworkConfig, if this isnt the right implementation, can you tell me which other way to init networkConfig?

the network config gets initialized by GetDefaultNetworkConfig, but then some fields are updated subsequently (if you take a look at NewSupersim). would suggest you just pass networkConfig to the NewAdminServer function

func NewAdminServer(log log.Logger, port uint64, networkConfig *config.NetworkConfig) *AdminServer {

jakim929 · 2024-11-13T16:56:19Z

admin/admin.go

+		NetworkConfig: s.networkConfig, // Ensure this is correctly set
+	}
+
+	if err := rpcServer.RegisterName("Admin", rpcMethods); err != nil {


convention is to lower case the namespace "admin"

s4rv4d · 2024-11-13T16:56:35Z

the network config is getting init by GetDefaultNetworkConfig, if this isnt the right implementation, can you tell me which other way to init networkConfig?

the network config gets initialized by GetDefaultNetworkConfig, but then some fields are updated subsequently (if you take a look at NewSupersim). would suggest you just pass networkConfig to the NewAdminServer function
func NewAdminServer(log log.Logger, port uint64, networkConfig *config.NetworkConfig) *AdminServer {

cool thanks for the reference making these changes

jakim929 · 2024-11-13T16:57:59Z

overall approach looks correct! sry for the delay in responding and lmk if u have other qs

s4rv4d · 2024-11-13T17:39:08Z

@jakim929 made the changes and updated admin_test as well

jakim929

left some comments, once those are fixed up it looks good to go!

jakim929 · 2024-11-13T17:40:53Z

admin/admin.go

@@ -38,7 +63,7 @@ func (s *AdminServer) Start(ctx context.Context) error {
 	addr := listener.Addr().(*net.TCPAddr)

 	s.port = uint64(addr.Port)
-	s.log.Debug("admin server listening", "port", s.port)
+	s.log.Debug("Admin server and RPC server listening on the same port", "port", s.port)


nit: admin server listening is probably fine. The admin server is just the RPC server except for a single endpoint

jakim929 · 2024-11-13T18:11:18Z

admin/admin.go

+		"L2CrossDomainMessenger": chain.L2Config.L1Addresses.L1CrossDomainMessengerProxy.String(),
+		"L2StandardBridge":       chain.L2Config.L1Addresses.L1StandardBridgeProxy.String(),
+	}
+	m.Log.Info("GetL2Addresses called", "chainId", *args)


this prob should just be a debug log

m.Log.Debug("admin_getL2Addresses)

jakim929 · 2024-11-13T18:14:18Z

admin/admin.go

+		"L2CrossDomainMessenger": chain.L2Config.L1Addresses.L1CrossDomainMessengerProxy.String(),
+		"L2StandardBridge":       chain.L2Config.L1Addresses.L1StandardBridgeProxy.String(),


these are named wrong - they should be named identically to the ones they refer to
L1CrossDomainMessengerProxy
L1StandardBridgeProxy

we should also add all of these?

AddressManager Address `json:"AddressManager" toml:"AddressManager"` L1CrossDomainMessengerProxy Address `json:"L1CrossDomainMessengerProxy" toml:"L1CrossDomainMessengerProxy"` L1ERC721BridgeProxy Address `json:"L1ERC721BridgeProxy" toml:"L1ERC721BridgeProxy"` L1StandardBridgeProxy Address `json:"L1StandardBridgeProxy" toml:"L1StandardBridgeProxy"` L2OutputOracleProxy Address `json:"L2OutputOracleProxy" toml:"L2OutputOracleProxy,omitempty"` OptimismMintableERC20FactoryProxy Address `json:"OptimismMintableERC20FactoryProxy" toml:"OptimismMintableERC20FactoryProxy"` OptimismPortalProxy Address `json:"OptimismPortalProxy,omitempty" toml:"OptimismPortalProxy,omitempty"` SystemConfigProxy Address `json:"SystemConfigProxy" toml:"SystemConfigProxy"` ProxyAdmin Address `json:"ProxyAdmin" toml:"ProxyAdmin"` SuperchainConfig Address `json:"SuperchainConfig,omitempty" toml:"SuperchainConfig,omitempty"`

jakim929 · 2024-11-13T18:14:43Z

admin/admin.go

+	rpcServer := rpc.NewServer()
+	rpcMethods := &RPCMethods{
+		Log:           s.log,
+		NetworkConfig: s.networkConfig, // Ensure this is correctly set


nit: remove comment

jakim929 · 2024-11-13T18:15:59Z

admin/admin.go

+	// Set up RPC server
+	rpcServer := rpc.NewServer()
+	rpcMethods := &RPCMethods{
+		Log:           s.log,
+		NetworkConfig: s.networkConfig, // Ensure this is correctly set
+	}
+
+	if err := rpcServer.RegisterName("admin", rpcMethods); err != nil {
+		return fmt.Errorf("failed to register RPC methods: %w", err)
+	}
+
+	// Mount the RPC server on the root path `/` to handle all RPC requests
+	router.Any("/", gin.WrapH(rpcServer))


we should move this logic to setupRouter?

yup makes sense

done✅

jakim929 · 2024-11-13T18:17:10Z

admin/admin.go

+	// Set up RPC server
+	rpcServer := rpc.NewServer()
+	rpcMethods := &RPCMethods{
+		Log:           s.log,
+		NetworkConfig: s.networkConfig, // Ensure this is correctly set
+	}
+
+	if err := rpcServer.RegisterName("admin", rpcMethods); err != nil {
+		return fmt.Errorf("failed to register RPC methods: %w", err)
+	}
+
+	// Mount the RPC server on the root path `/` to handle all RPC requests
+	router.Any("/", gin.WrapH(rpcServer))


we should stick to just POST requests here

router.POST

jakim929 · 2024-11-13T18:18:41Z

admin/admin.go

+func (m *RPCMethods) GetL1Addresses(args *uint64) *map[string]string {
+	chain := filterByChainID(m.NetworkConfig.L2Configs, *args)
+	if chain == nil {
+		m.Log.Info("chain not found")


should be m.Log.Debug

jakim929 · 2024-11-13T18:18:50Z

admin/admin.go

+
+func (m *RPCMethods) GetConfig(args *struct{}) *config.NetworkConfig {
+
+	m.Log.Info("GetConfig called")


nit: should be m.Log.Debug

jakim929 · 2024-11-13T18:19:50Z

admin/admin.go

+	var b strings.Builder
+	fmt.Fprintf(&b, "Admin Server: %s\n\n", s.Endpoint())
+	return b.String()


nit: doesn't need to be a string builder because it's a single interpolation.

jakim929 · 2024-11-13T18:20:40Z

admin/admin.go

@@ -19,15 +21,38 @@ type AdminServer struct {
 	cancel context.CancelFunc
 	wg     sync.WaitGroup

+	rpcServer     *rpc.Server // New field for the RPC server


nit: unnecessary comment

s4rv4d · 2024-11-13T18:47:59Z

@jakim929 made the necessary review changes

jakim929 · 2024-11-13T18:50:03Z

admin/admin_test.go

+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	go server.Start(ctx)
+	time.Sleep(1 * time.Second) // Allow time for the server to start


can we replace this with WaitForWithTimeout? time.Sleep makes tests more flaky and adds to time

yes sir we can, making this change

jakim929

few last changes! will keep an eye so once those are done I can approve - no need to re-ping

admin/admin.go

supersim.go

added getL1Addresses(chainId) support

450707c

s4rv4d requested a review from a team as a code owner November 8, 2024 18:11

setup RPC server

1e466a8

s4rv4d added 3 commits November 13, 2024 13:06

struct edit

bf59b69

small change

5aa660a

added getL1Addresses

a51096d

jakim929 reviewed Nov 13, 2024

View reviewed changes

made review changes and updated admin_test.go

4fd49c6

s4rv4d changed the title ~~feat(admin): added getL1Addresses(chainId) support with /getL1Addresses/:chainID~~ feat(admin): added getL1Addresses(chainId) rpc support Nov 13, 2024

jakim929 requested changes Nov 13, 2024

View reviewed changes

review updates

1ef9acd

jakim929 reviewed Nov 13, 2024

View reviewed changes

log.debug to log.error

66f4b56

jakim929 requested changes Nov 13, 2024

View reviewed changes

admin/admin.go Outdated Show resolved Hide resolved

supersim.go Outdated Show resolved Hide resolved

s4rv4d and others added 2 commits November 14, 2024 00:39

review changes

ded5b96

Merge branch 'main' into main

585f173

jakim929 approved these changes Nov 13, 2024

View reviewed changes

jakim929 merged commit 9890af2 into ethereum-optimism:main Nov 13, 2024
2 of 4 checks passed

		"L2CrossDomainMessenger": chain.L2Config.L1Addresses.L1CrossDomainMessengerProxy.String(),
		"L2StandardBridge": chain.L2Config.L1Addresses.L1StandardBridgeProxy.String(),


		func (m RPCMethods) GetConfig(args struct{}) *config.NetworkConfig {

		m.Log.Info("GetConfig called")

feat(admin): added getL1Addresses(chainId) rpc support #254

feat(admin): added getL1Addresses(chainId) rpc support #254

Conversation

s4rv4d commented Nov 8, 2024

s4rv4d commented Nov 8, 2024

jakim929 commented Nov 9, 2024

s4rv4d commented Nov 9, 2024 • edited Loading

s4rv4d commented Nov 12, 2024

s4rv4d commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakim929 commented Nov 13, 2024

Choose a reason for hiding this comment

s4rv4d commented Nov 13, 2024

jakim929 commented Nov 13, 2024

s4rv4d commented Nov 13, 2024 • edited Loading

jakim929 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

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

s4rv4d commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakim929 left a comment

Choose a reason for hiding this comment

s4rv4d commented Nov 9, 2024 •

edited

Loading

s4rv4d commented Nov 13, 2024 •

edited

Loading