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

feat: wire v2 handlers #22112

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
3 changes: 2 additions & 1 deletion runtime/v2/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ require (
cosmossdk.io/store/v2 v2.0.0-00010101000000-000000000000
cosmossdk.io/x/tx v0.13.3
github.com/cosmos/gogoproto v1.7.0
github.com/stretchr/testify v1.9.0
google.golang.org/grpc v1.67.1
google.golang.org/protobuf v1.34.2
)
Expand Down Expand Up @@ -70,7 +71,7 @@ require (
github.com/rogpeppe/go-internal v1.12.0 // indirect
github.com/rs/zerolog v1.33.0 // indirect
github.com/spf13/cast v1.7.0 // indirect
github.com/stretchr/testify v1.9.0 // indirect
github.com/stretchr/objx v0.5.2 // indirect
github.com/syndtr/goleveldb v1.0.1-0.20220721030215-126854af5e6d // indirect
github.com/tendermint/go-amino v0.16.0 // indirect
github.com/tidwall/btree v1.7.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions runtime/v2/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,8 @@ github.com/spf13/cast v1.7.0 h1:ntdiHjuueXFgm5nzDRdOS4yfT43P5Fnud6DH50rz/7w=
github.com/spf13/cast v1.7.0/go.mod h1:ancEpBxwJDODSW/UG4rDrAqiKolqNNh2DX3mk86cAdo=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY=
github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA=
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
Expand Down
50 changes: 50 additions & 0 deletions runtime/v2/services_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package runtime

import (
"testing"

appmodulev2 "cosmossdk.io/core/appmodule/v2"
"cosmossdk.io/core/transaction"
"cosmossdk.io/server/v2/stf"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)

// MockModule implements both HasMsgHandlers and HasQueryHandlers
type MockModule struct {
mock.Mock
appmodulev2.AppModule
}

func (m *MockModule) RegisterMsgHandlers(router appmodulev2.MsgRouter) {
m.Called(router)
}

func (m *MockModule) RegisterQueryHandlers(router appmodulev2.QueryRouter) {
m.Called(router)
}

func TestRegisterServices(t *testing.T) {
mockModule := new(MockModule)

app := &App[transaction.Tx]{
msgRouterBuilder: stf.NewMsgRouterBuilder(),
queryRouterBuilder: stf.NewMsgRouterBuilder(),
}

mm := &MM[transaction.Tx]{
modules: map[string]appmodulev2.AppModule{
"mock": mockModule,
},
}

mockModule.On("RegisterMsgHandlers", app.msgRouterBuilder).Once()
mockModule.On("RegisterQueryHandlers", app.queryRouterBuilder).Once()

err := mm.RegisterServices(app)

assert.NoError(t, err)

mockModule.AssertExpectations(t)
}
18 changes: 18 additions & 0 deletions server/v2/api/rest/config.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package rest

func DefaultConfig() *Config {
return &Config{
Enable: true,
Address: "localhost:8080",
}
}

type CfgOption func(*Config)
Copy link
Member

Choose a reason for hiding this comment

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

There's no option defined, so let's delete this.


// Config defines configuration for the HTTP server.
type Config struct {
// Enable defines if the HTTP server should be enabled.
Copy link
Member

Choose a reason for hiding this comment

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

can we say rest server instead of http server? so it matches the component name.

Enable bool `mapstructure:"enable" toml:"enable" comment:"Enable defines if the HTTP server should be enabled."`
// Address defines the API server to listen on
Address string `mapstructure:"address" toml:"address" comment:"Address defines the HTTP server address to bind to."`
}
60 changes: 60 additions & 0 deletions server/v2/api/rest/handler.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package rest

import (
"cosmossdk.io/core/transaction"
"cosmossdk.io/server/v2/appmanager"
"encoding/json"
"fmt"
gogoproto "github.com/cosmos/gogoproto/proto"
"github.com/gogo/protobuf/jsonpb"
"net/http"
"reflect"
Fixed Show fixed Hide fixed

Check notice

Code scanning / CodeQL

Sensitive package import Note

Certain system packages contain functions which may be a possible source of non-determinism
"strings"
)

const (
ContentTypeJSON = "application/json"
ContentTypeOctetStream = "application/octet-stream"
ContentTypeProtobuf = "application/x-protobuf"
)

type DefaultHandler[T transaction.Tx] struct {
appManager *appmanager.AppManager[T]
}

func (h *DefaultHandler[T]) ServeHTTP(w http.ResponseWriter, r *http.Request) {
path := strings.TrimPrefix(r.URL.Path, "/")

if r.Method != http.MethodPost {
http.Error(w, "Method not allowed", http.StatusMethodNotAllowed)
return
}

contentType := r.Header.Get("Content-Type")
if contentType != ContentTypeJSON {
contentType = ContentTypeJSON
}
Comment on lines +37 to +40
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate Content-Type header instead of defaulting

Currently, if the Content-Type is not application/json, the handler defaults it to application/json. This may lead to unexpected behavior if the client sends data with an incorrect content type.

Consider validating the Content-Type and returning a 415 Unsupported Media Type error if it's not application/json:

 contentType := r.Header.Get("Content-Type")
 if contentType != ContentTypeJSON {
-    contentType = ContentTypeJSON
+    http.Error(w, "Unsupported Media Type", http.StatusUnsupportedMediaType)
+    return
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
contentType := r.Header.Get("Content-Type")
if contentType != ContentTypeJSON {
contentType = ContentTypeJSON
}
contentType := r.Header.Get("Content-Type")
if contentType != ContentTypeJSON {
http.Error(w, "Unsupported Media Type", http.StatusUnsupportedMediaType)
return
}


requestType := gogoproto.MessageType(path)
if requestType == nil {
http.Error(w, "Unknown request type", http.StatusNotFound)
return
}

msg := reflect.New(requestType.Elem()).Interface().(gogoproto.Message)
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Minimize the use of reflection for better performance and safety

Reflection can introduce performance overhead and potential runtime errors. According to the Uber Go Style Guide, reflection should be used carefully.

Consider alternative approaches to create new message instances without reflection. For example, maintain a registry or map of constructor functions keyed by message type.


err := jsonpb.Unmarshal(r.Body, msg)
if err != nil {
http.Error(w, "Error parsing body", http.StatusBadRequest)
fmt.Fprintf(w, "Error parsing body: %v\n", err)
return
}
Comment on lines +51 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consolidate error handling when parsing the request body

Using both http.Error and fmt.Fprintf to write error responses can result in malformed HTTP responses because headers may have already been sent. Instead, send a single, well-formatted error response.

Modify the error handling to return a JSON response with error details:

 if err != nil {
-    http.Error(w, "Error parsing body", http.StatusBadRequest)
-    fmt.Fprintf(w, "Error parsing body: %v\n", err)
+    w.Header().Set("Content-Type", ContentTypeJSON)
+    w.WriteHeader(http.StatusBadRequest)
+    json.NewEncoder(w).Encode(map[string]string{
+        "error": fmt.Sprintf("Error parsing body: %v", err),
+    })
     return
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if err != nil {
http.Error(w, "Error parsing body", http.StatusBadRequest)
fmt.Fprintf(w, "Error parsing body: %v\n", err)
return
}
if err != nil {
w.Header().Set("Content-Type", ContentTypeJSON)
w.WriteHeader(http.StatusBadRequest)
json.NewEncoder(w).Encode(map[string]string{
"error": fmt.Sprintf("Error parsing body: %v", err),
})
return
}


query, err := h.appManager.Query(r.Context(), 0, msg)
if err != nil {
http.Error(w, "Error querying", http.StatusInternalServerError)
return
}

json.NewEncoder(w).Encode(query)
}
40 changes: 40 additions & 0 deletions server/v2/api/rest/handler_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package rest

import (
"bytes"
"cosmossdk.io/core/transaction"
"encoding/json"
"net/http"
"net/http/httptest"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)

func TestDefaultHandlerServeHTTP(t *testing.T) {
mockAppManager := new(MockAppManager)
handler := &DefaultHandler[transaction.Tx]{
appManager: mockAppManager,
}

body := []byte(`{"test": "data"}`)
req, err := http.NewRequest("POST", "/MockMessage", bytes.NewBuffer(body))
assert.NoError(t, err)

rr := httptest.NewRecorder()

expectedResponse := map[string]string{"result": "success"}
mockAppManager.On("Query", mock.Anything, int64(0), mock.AnythingOfType("*rest.MockMessage")).Return(expectedResponse, nil)

handler.ServeHTTP(rr, req)

assert.Equal(t, http.StatusOK, rr.Code)

var response map[string]string
err = json.Unmarshal(rr.Body.Bytes(), &response)
assert.NoError(t, err)
assert.Equal(t, expectedResponse, response)

mockAppManager.AssertExpectations(t)
}
93 changes: 93 additions & 0 deletions server/v2/api/rest/server.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package rest

import (
"context"
"cosmossdk.io/server/v2/appmanager"
"errors"
"net/http"

"github.com/gorilla/mux"
randygrok marked this conversation as resolved.
Show resolved Hide resolved

"cosmossdk.io/core/transaction"
"cosmossdk.io/log"
serverv2 "cosmossdk.io/server/v2"
)

const (
ServerName = "rest-v2"
)

type Server[T transaction.Tx] struct {
logger log.Logger
router *mux.Router

httpServer *http.Server
config *Config
cfgOptions []CfgOption
}

func New[T transaction.Tx](cfgOptions ...CfgOption) *Server[T] {
return &Server[T]{
cfgOptions: cfgOptions,
}
}

func (s *Server[T]) Name() string {
return ServerName
}

func (s *Server[T]) Init(appI serverv2.AppI[T], cfg map[string]any, logger log.Logger) error {
s.logger = logger.With(log.ModuleKey, s.Name())

s.config = s.Config().(*Config)
Copy link
Member

Choose a reason for hiding this comment

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

we need the same config unmarshalling than with other servers here.


var appManager *appmanager.AppManager[T]
appManager = appI.GetAppManager()

s.router = mux.NewRouter()
s.router.PathPrefix("/").Handler(&DefaultHandler[T]{
appManager: appManager,
})

return nil
}

func (s *Server[T]) Start(ctx context.Context) error {
s.httpServer = &http.Server{
Copy link
Member

Choose a reason for hiding this comment

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

We need to add the enable check like we do in other servers here.

Addr: s.config.Address,
Handler: s.router,
}
Comment on lines +51 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Check if the server is enabled before starting

Currently, the Start() method starts the HTTP server without checking whether the server is enabled in the configuration. This could lead to the server running when it shouldn't be.

Apply this diff to add the enable check:

 func (s *Server[T]) Start(ctx context.Context) error {
+	if !s.config.Enable {
+		s.logger.Info("HTTP server is disabled")
+		return nil
+	}
 
 	s.httpServer = &http.Server{
 		Addr:    s.config.Address,
 		Handler: s.router,
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (s *Server[T]) Start(ctx context.Context) error {
s.httpServer = &http.Server{
Addr: s.config.Address,
Handler: s.router,
}
func (s *Server[T]) Start(ctx context.Context) error {
if !s.config.Enable {
s.logger.Info("HTTP server is disabled")
return nil
}
s.httpServer = &http.Server{
Addr: s.config.Address,
Handler: s.router,
}


go func() {
Copy link
Member

Choose a reason for hiding this comment

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

no need to be in a go routine, we can be blocking, server components are already running in a go routine

s.logger.Info("Starting HTTP server", "address", s.config.Address)
if err := s.httpServer.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
s.logger.Error("Failed to start HTTP server", "error", err)
}
Comment on lines +59 to +61
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Notify the caller if the HTTP server fails to start

In the goroutine, if ListenAndServe() returns an error other than http.ErrServerClosed, the error is only logged but not returned to the caller. This means the caller has no way of knowing that the server failed to start.

Consider modifying the Start() method to capture the error and return it to the caller. Here's how you might adjust the code:

 func (s *Server[T]) Start(ctx context.Context) error {
+	errChan := make(chan error, 1)
 
 	s.httpServer = &http.Server{
 		Addr:    s.config.Address,
 		Handler: s.router,
 	}
 
 	go func() {
 		s.logger.Info("Starting HTTP server", "address", s.config.Address)
 		if err := s.httpServer.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
 			s.logger.Error("Failed to start HTTP server", "error", err)
+			errChan <- err
 		} else {
+			errChan <- nil
 		}
 	}()
 
+	if err := <-errChan; err != nil {
+		return err
+	}
 
 	return nil
 }

This way, the Start() method will wait for the server to start and report any errors back to the caller.

Committable suggestion was skipped due to low confidence.

}()
Comment on lines +57 to +62

Check notice

Code scanning / CodeQL

Spawning a Go routine Note

Spawning a Go routine may be a possible source of non-determinism

return nil
}

func (s *Server[T]) Stop(ctx context.Context) error {
if !s.config.Enable {
return nil
}

s.logger.Info("Stopping HTTP server")

return s.httpServer.Shutdown(ctx)
}

func (s *Server[T]) Config() any {
if s.config == nil || s.config == (&Config{}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the comparison in the Config() method

In line 78, the comparison s.config == (&Config{}) is comparing the pointer s.config to a new pointer &Config{}. This will always evaluate to false because they point to different addresses. Instead, you should compare the value that s.config points to with an empty Config struct to check if it is uninitialized.

Apply this diff to fix the comparison:

 func (s *Server[T]) Config() any {
-	if s.config == nil || s.config == (&Config{}) {
+	if s.config == nil || *s.config == (Config{}) {
 		cfg := DefaultConfig()
 
 		for _, opt := range s.cfgOptions {
 			opt(cfg)
 		}
 
 		return cfg
 	}
 
 	return s.config
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if s.config == nil || s.config == (&Config{}) {
if s.config == nil || *s.config == (Config{}) {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if s.config == nil || s.config == (&Config{}) {
if s.config == nil || s.config.Address == "" {

bot is right

cfg := DefaultConfig()

for _, opt := range s.cfgOptions {
opt(cfg)
}

return cfg
}

return s.config
}
46 changes: 46 additions & 0 deletions server/v2/api/rest/server_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package rest

import (
"testing"

"github.com/stretchr/testify/require"

"cosmossdk.io/core/transaction"
)

func TestServerConfig(t *testing.T) {
testCases := []struct {
name string
setupFunc func() *Config
expectedConfig *Config
}{
{
name: "Default configuration, no custom configuration",
setupFunc: func() *Config {
s := New[transaction.Tx]()
return s.Config().(*Config)
},
expectedConfig: DefaultConfig(),
},
{
name: "Custom configuration",
setupFunc: func() *Config {
s := New[transaction.Tx](func(config *Config) {
config.Enable = false
})
return s.Config().(*Config)
},
expectedConfig: &Config{
Enable: false, // Custom configuration
Address: "localhost:8080",
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
config := tc.setupFunc()
require.Equal(t, tc.expectedConfig, config)
})
}
}
2 changes: 2 additions & 0 deletions simapp/v2/simdv2/cmd/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
runtimev2 "cosmossdk.io/runtime/v2"
serverv2 "cosmossdk.io/server/v2"
"cosmossdk.io/server/v2/api/grpc"
"cosmossdk.io/server/v2/api/rest"
"cosmossdk.io/server/v2/api/telemetry"
"cosmossdk.io/server/v2/cometbft"
"cosmossdk.io/server/v2/store"
Expand Down Expand Up @@ -83,6 +84,7 @@ func initRootCmd[T transaction.Tx](
grpc.New[T](),
store.New[T](newApp),
telemetry.New[T](),
rest.New[T](),
); err != nil {
panic(err)
}
Expand Down
1 change: 0 additions & 1 deletion x/bank/v2/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package keeper

import (
"context"

"cosmossdk.io/collections"
"cosmossdk.io/collections/indexes"
"cosmossdk.io/core/address"
Expand Down
Loading