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

Merged
merged 32 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
233a249
server v2 rest for new modules
Sep 30, 2024
d7202e4
server config with defaults
Sep 30, 2024
f1d7ebe
pre msgrouter
Sep 30, 2024
75e1924
use handlers
Oct 1, 2024
86d450f
it queries
Oct 2, 2024
f0af908
lint
Oct 3, 2024
22b9f92
Merge remote-tracking branch 'origin/main' into feat/wire-v2-handlers
Oct 3, 2024
45b5d74
Merge remote-tracking branch 'origin/main' into feat/wire-v2-handlers
Oct 3, 2024
a67fcc9
include README
Oct 4, 2024
3007e67
create constructor
Oct 4, 2024
b4027dd
add lint
Oct 4, 2024
85bd55a
Merge branch 'main' into feat/wire-v2-handlers
randygrok Oct 7, 2024
f22a135
remove mux usage
Oct 7, 2024
5d05234
Merge branch 'feat/wire-v2-handlers' of 6github.com-randy:cosmos/cosmo…
Oct 7, 2024
ee8d0a4
go mod tidy
Oct 7, 2024
40b5bae
Merge branch 'main' into feat/wire-v2-handlers
randygrok Oct 8, 2024
52d57cf
Merge branch 'main' into feat/wire-v2-handlers
randygrok Oct 8, 2024
a733c83
changes due to review
Oct 10, 2024
8d78a2c
Merge branch 'feat/wire-v2-handlers' of 6github.com-randy:cosmos/cosmo…
Oct 10, 2024
8dc09a3
add enable flag
Oct 10, 2024
c186af2
fix config check
Oct 10, 2024
2cf4c62
add some config options
Oct 10, 2024
9e6b8fb
remove Fprintf
Oct 10, 2024
ac25a49
improve part of the code
Oct 10, 2024
9dd5c9f
refactor a little
Oct 10, 2024
2b028fd
Merge branch 'main' into feat/wire-v2-handlers
randygrok Oct 14, 2024
1c7d32c
use cosmos jsonpb
Oct 14, 2024
506267d
unmarshal server config
Oct 14, 2024
a8a173e
Merge branch 'main' into feat/wire-v2-handlers
randygrok Oct 16, 2024
3608feb
make lint and comments from review
Oct 16, 2024
c700ad9
Merge branch 'feat/wire-v2-handlers' of 6github.com-randy:cosmos/cosmo…
Oct 16, 2024
db3ac74
lint fix
julienrbrt Oct 16, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)

// Config defines configuration for the HTTP server.
type Config struct {
// Enable defines if the HTTP server should be enabled.
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"

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
}
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
}
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
Copy link
Member

@julienrbrt julienrbrt Oct 15, 2024

Choose a reason for hiding this comment

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

super nit, for consistency for the other, can we get the logger error lowercase?
additionally, could you fix conflicts?

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"

"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)

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{
Addr: s.config.Address,
Handler: s.router,
}
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() {
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)
}
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.

}()

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{}) {

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