From 3085d91649cce897b1f795ad880a34e9fba2793d Mon Sep 17 00:00:00 2001 From: otherview Date: Fri, 13 Oct 2023 15:18:08 +0100 Subject: [PATCH 1/4] Normalizing OG errors --- .../obscurogateway/obscurogateway_test.go | 79 +++++++- .../accountmanager/account_manager.go | 6 +- tools/walletextension/api/routes.go | 168 ++++++++---------- tools/walletextension/api/utils.go | 42 +++++ tools/walletextension/common/constants.go | 8 +- tools/walletextension/common/json.go | 20 +++ tools/walletextension/userconn/user_conn.go | 64 +------ tools/walletextension/wallet_extension.go | 24 +-- 8 files changed, 234 insertions(+), 177 deletions(-) create mode 100644 tools/walletextension/common/json.go diff --git a/integration/obscurogateway/obscurogateway_test.go b/integration/obscurogateway/obscurogateway_test.go index f6cd9e3862..8cf90c42e3 100644 --- a/integration/obscurogateway/obscurogateway_test.go +++ b/integration/obscurogateway/obscurogateway_test.go @@ -2,6 +2,7 @@ package faucet import ( "context" + "encoding/json" "fmt" "math/big" "net/http" @@ -9,11 +10,15 @@ import ( "testing" "time" + wecommon "github.com/obscuronet/go-obscuro/tools/walletextension/common" + gethcommon "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/ethclient" "github.com/ethereum/go-ethereum/log" "github.com/obscuronet/go-obscuro/go/common" + "github.com/obscuronet/go-obscuro/go/common/httputil" + "github.com/obscuronet/go-obscuro/go/enclave/genesis" "github.com/obscuronet/go-obscuro/go/wallet" "github.com/obscuronet/go-obscuro/integration" "github.com/obscuronet/go-obscuro/integration/common/testlog" @@ -44,7 +49,7 @@ const ( func TestObscuroGateway(t *testing.T) { startPort := integration.StartPortObscuroGatewayUnitTest - wallets := createObscuroNetwork(t, startPort) + createObscuroNetwork(t, startPort) obscuroGatewayConf := config.Config{ WalletExtensionHost: "127.0.0.1", @@ -76,12 +81,30 @@ func TestObscuroGateway(t *testing.T) { err := waitServerIsReady(httpURL) require.NoError(t, err) - // join + register against the og + // run the tests against the exis + for name, test := range map[string]func(*testing.T, string, string){ + "testAreTxsMinted": testAreTxsMinted, + "testErrorHandling": testErrorHandling, + } { + t.Run(name, func(t *testing.T) { + test(t, httpURL, wsURL) + }) + } + + // Gracefully shutdown + err = obscuroGwContainer.Stop() + assert.NoError(t, err) +} + +func testAreTxsMinted(t *testing.T, httpURL, wsURL string) { + // set up the ogClient ogClient := lib.NewObscuroGatewayLibrary(httpURL, wsURL) - err = ogClient.Join() + + // join + register against the og + err := ogClient.Join() require.NoError(t, err) - w := wallets.L2FaucetWallet + w := wallet.NewInMemoryWalletFromConfig(genesis.TestnetPrefundedPK, integration.ObscuroChainID, testlog.Logger()) err = ogClient.RegisterAccount(w.PrivateKey(), w.Address()) require.NoError(t, err) @@ -99,10 +122,49 @@ func TestObscuroGateway(t *testing.T) { receipt, err := ethStdClient.TransactionReceipt(context.Background(), txHash) assert.NoError(t, err) require.True(t, receipt.Status == 1) +} - // Gracefully shutdown - err = obscuroGwContainer.Stop() - assert.NoError(t, err) +func testErrorHandling(t *testing.T, httpURL, wsURL string) { + // set up the ogClient + ogClient := lib.NewObscuroGatewayLibrary(httpURL, wsURL) + + // join + register against the og + err := ogClient.Join() + require.NoError(t, err) + + // register an account + w := wallet.NewInMemoryWalletFromConfig(genesis.TestnetPrefundedPK, integration.ObscuroChainID, testlog.Logger()) + err = ogClient.RegisterAccount(w.PrivateKey(), w.Address()) + require.NoError(t, err) + + // make requests to geth for comparison + + for _, req := range []string{ + `{"jsonrpc":"2.0","method":"eth_getBalance","params":["0xA58C60cc047592DE97BF1E8d2f225Fc5D959De77", "latest"],"id":1}`, + `{"jsonrpc":"2.0","method":"eth_getBalance","params":[],"id":1}`, + `{"jsonrpc":"2.0","method":"eth_getgetget","params":["0xA58C60cc047592DE97BF1E8d2f225Fc5D959De77", "latest"],"id":1}`, + `{"method":"eth_getBalance","params":["0xA58C60cc047592DE97BF1E8d2f225Fc5D959De77", "latest"],"id":1}`, + `{"jsonrpc":"2.0","method":"eth_getBalance","params":["0xA58C60cc047592DE97BF1E8d2f225Fc5D959De77", "latest"],"id":1,"extra":"extra_field"}`, + `{"jsonrpc":"2.0","method":"eth_sendTransaction","params":[["0xA58C60cc047592DE97BF1E8d2f225Fc5D959De77", "0x1234"]],"id":1}`, + } { + // ensure the geth request is issued correctly (should return 200 ok with jsonRPCError) + _, response, err := httputil.PostDataJSON(ogClient.HTTP(), []byte(req)) + require.NoError(t, err) + + // unmarshall the response to JSONRPCMessage + jsonRPCError := wecommon.JSONRPCMessage{} + err = json.Unmarshal(response, &jsonRPCError) + require.NoError(t, err) + + // repeat the process for the gateway + _, response, err = httputil.PostDataJSON(fmt.Sprintf("http://localhost:%d", integration.StartPortObscuroGatewayUnitTest), []byte(req)) + require.NoError(t, err) + + // we only care about format + jsonRPCError = wecommon.JSONRPCMessage{} + err = json.Unmarshal(response, &jsonRPCError) + require.NoError(t, err) + } } func transferRandomAddr(t *testing.T, client *ethclient.Client, w wallet.Wallet) common.TxHash { @@ -160,7 +222,7 @@ func transferRandomAddr(t *testing.T, client *ethclient.Client, w wallet.Wallet) } // Creates a single-node Obscuro network for testing. -func createObscuroNetwork(t *testing.T, startPort int) *params.SimWallets { +func createObscuroNetwork(t *testing.T, startPort int) { // Create the Obscuro network. numberOfNodes := 1 wallets := params.NewSimWallets(1, numberOfNodes, integration.EthereumChainID, integration.ObscuroChainID) @@ -180,7 +242,6 @@ func createObscuroNetwork(t *testing.T, startPort int) *params.SimWallets { if err != nil { panic(fmt.Sprintf("failed to create test Obscuro network. Cause: %s", err)) } - return wallets } func waitServerIsReady(serverAddr string) error { diff --git a/tools/walletextension/accountmanager/account_manager.go b/tools/walletextension/accountmanager/account_manager.go index 235acdd1f7..56a69edbfc 100644 --- a/tools/walletextension/accountmanager/account_manager.go +++ b/tools/walletextension/accountmanager/account_manager.go @@ -312,7 +312,11 @@ func (m *AccountManager) executeSubscribe(client rpc.Client, req *RPCRequest, re case err = <-subscription.Err(): // An error on this channel means the subscription has ended, so we exit the loop. if userConn != nil && err != nil { - userConn.HandleError(err.Error()) + // todo properly handle the disconnect to the user side + err = userConn.WriteResponse([]byte(err.Error())) + if err != nil { + m.logger.Error("unable to close connection with the user", log.ErrKey, err) + } } return diff --git a/tools/walletextension/api/routes.go b/tools/walletextension/api/routes.go index d3244622f2..4832ad279f 100644 --- a/tools/walletextension/api/routes.go +++ b/tools/walletextension/api/routes.go @@ -6,8 +6,9 @@ import ( "fmt" "net/http" - "github.com/obscuronet/go-obscuro/go/common/httputil" "github.com/obscuronet/go-obscuro/go/common/log" + + "github.com/obscuronet/go-obscuro/go/common/httputil" "github.com/obscuronet/go-obscuro/go/rpc" "github.com/obscuronet/go-obscuro/tools/walletextension" "github.com/obscuronet/go-obscuro/tools/walletextension/common" @@ -142,48 +143,45 @@ func wsRequestHandler(walletExt *walletextension.WalletExtension, resp http.Resp func ethRequestHandler(walletExt *walletextension.WalletExtension, conn userconn.UserConn) { body, err := conn.ReadRequest() if err != nil { - err = fmt.Errorf("error reading request: %w", err) - conn.HandleError(err.Error()) - walletExt.Logger().Error(err.Error()) + handleEthError(nil, conn, walletExt.Logger(), fmt.Errorf("error reading request - %w", err)) return } request, err := parseRequest(body) if err != nil { - conn.HandleError(err.Error()) + handleError(conn, walletExt.Logger(), err) return } walletExt.Logger().Debug("REQUEST", "method", request.Method, "body", string(body)) if request.Method == rpc.Subscribe && !conn.SupportsSubscriptions() { - conn.HandleError(common.ErrSubscribeFailHTTP) + handleError(conn, walletExt.Logger(), fmt.Errorf("received an %s request but the connection does not support subscriptions", rpc.Subscribe)) return } // Get userID hexUserID, err := getUserID(conn, 1) if err != nil || !walletExt.UserExists(hexUserID) { - walletExt.Logger().Error(fmt.Errorf("user not found in the query params: %w. Using the default user", err).Error()) + walletExt.Logger().Error("user not found in the query params: %w. Using the default user", log.ErrKey, err) hexUserID = hex.EncodeToString([]byte(common.DefaultUser)) // todo (@ziga) - this can be removed once old WE endpoints are removed } // todo (@pedro) remove this conn dependency response, err := walletExt.ProxyEthRequest(request, conn, hexUserID) if err != nil { - walletExt.Logger().Error("error while proxying request", log.ErrKey, err) - response = common.CraftErrorResponse(err) + handleEthError(request, conn, walletExt.Logger(), err) + return } rpcResponse, err := json.Marshal(response) if err != nil { - conn.HandleError(fmt.Sprintf("failed to remarshal RPC response to return to caller: %s", err)) + handleEthError(request, conn, walletExt.Logger(), err) return } walletExt.Logger().Info(fmt.Sprintf("Forwarding %s response from Obscuro node: %s", request.Method, rpcResponse)) - err = conn.WriteResponse(rpcResponse) - if err != nil { - return + if err = conn.WriteResponse(rpcResponse); err != nil { + walletExt.Logger().Error("error writing success response", log.ErrKey, err) } } @@ -194,16 +192,14 @@ func readyRequestHandler(_ *walletextension.WalletExtension, _ userconn.UserConn func generateViewingKeyRequestHandler(walletExt *walletextension.WalletExtension, conn userconn.UserConn) { body, err := conn.ReadRequest() if err != nil { - err = fmt.Errorf("error reading request: %w", err) - conn.HandleError(err.Error()) - walletExt.Logger().Error(err.Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("error reading request: %w", err)) return } var reqJSONMap map[string]string err = json.Unmarshal(body, &reqJSONMap) if err != nil { - conn.HandleError(fmt.Sprintf("could not unmarshal address request - %s", err)) + handleError(conn, walletExt.Logger(), fmt.Errorf("could not unmarshal address request - %w", err)) return } @@ -211,87 +207,82 @@ func generateViewingKeyRequestHandler(walletExt *walletextension.WalletExtension pubViewingKey, err := walletExt.GenerateViewingKey(address) if err != nil { - conn.HandleError(fmt.Sprintf("unable to generate vieweing key: %s", err)) + handleError(conn, walletExt.Logger(), fmt.Errorf("unable to generate vieweing key - %w", err)) return } err = conn.WriteResponse([]byte(pubViewingKey)) if err != nil { - return + walletExt.Logger().Error("error writing success response", log.ErrKey, err) } } // submitViewingKeyRequestHandler submits the viewing key and signed bytes to the WE -func submitViewingKeyRequestHandler(walletExt *walletextension.WalletExtension, userConn userconn.UserConn) { - body, err := userConn.ReadRequest() +func submitViewingKeyRequestHandler(walletExt *walletextension.WalletExtension, conn userconn.UserConn) { + body, err := conn.ReadRequest() if err != nil { - userConn.HandleError("Error: bad request") - walletExt.Logger().Error(fmt.Errorf("error reading request: %w", err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("error reading request: %w", err)) return } var reqJSONMap map[string]string err = json.Unmarshal(body, &reqJSONMap) if err != nil { - userConn.HandleError(fmt.Sprintf("could not unmarshal address and signature from client to JSON: %s", err)) + handleError(conn, walletExt.Logger(), fmt.Errorf("could not unmarshal address request - %w", err)) return } accAddress := gethcommon.HexToAddress(reqJSONMap[common.JSONKeyAddress]) signature, err := hex.DecodeString(reqJSONMap[common.JSONKeySignature][2:]) if err != nil { - userConn.HandleError(fmt.Sprintf("could not decode signature from client to hex: %s", err)) + handleError(conn, walletExt.Logger(), fmt.Errorf("could not decode signature from client to hex - %w", err)) return } err = walletExt.SubmitViewingKey(accAddress, signature) if err != nil { - userConn.HandleError(fmt.Sprintf("could not submit viewing key - %s", err)) + handleError(conn, walletExt.Logger(), fmt.Errorf("could not submit viewing key - %w", err)) return } - err = userConn.WriteResponse([]byte(common.SuccessMsg)) + err = conn.WriteResponse([]byte(common.SuccessMsg)) if err != nil { - walletExt.Logger().Error(fmt.Errorf("error writing success response, %w", err).Error()) + walletExt.Logger().Error("error writing success response", log.ErrKey, err) return } } // This function handles request to /join endpoint. It is responsible to create new user (new key-pair) and store it to the db -func joinRequestHandler(walletExt *walletextension.WalletExtension, userConn userconn.UserConn) { +func joinRequestHandler(walletExt *walletextension.WalletExtension, conn userconn.UserConn) { // todo (@ziga) add protection against DDOS attacks - _, err := userConn.ReadRequest() + _, err := conn.ReadRequest() if err != nil { - userConn.HandleError("Error: bad request") - walletExt.Logger().Error(fmt.Errorf("error reading request: %w", err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("error reading request: %w", err)) return } // generate new key-pair and store it in the database hexUserID, err := walletExt.GenerateAndStoreNewUser() if err != nil { - userConn.HandleError("Internal error") - walletExt.Logger().Error(fmt.Errorf("error creating new user, %w", err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("internal Error")) + walletExt.Logger().Error("error creating new user", log.ErrKey, err) } // write hex encoded userID in the response - err = userConn.WriteResponse([]byte(hexUserID)) - + err = conn.WriteResponse([]byte(hexUserID)) if err != nil { - walletExt.Logger().Error(fmt.Errorf("error writing success response, %w", err).Error()) - return + walletExt.Logger().Error("error writing success response", log.ErrKey, err) } } // This function handles request to /authenticate endpoint. // In the request we receive message, signature and address in JSON as request body and userID and address as query parameters // We then check if message is in correct format and if signature is valid. If all checks pass we save address and signature against userID -func authenticateRequestHandler(walletExt *walletextension.WalletExtension, userConn userconn.UserConn) { +func authenticateRequestHandler(walletExt *walletextension.WalletExtension, conn userconn.UserConn) { // read the request - body, err := userConn.ReadRequest() + body, err := conn.ReadRequest() if err != nil { - userConn.HandleError("Error: bad request") - walletExt.Logger().Error(fmt.Errorf("error reading request: %w", err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("error reading request: %w", err)) return } @@ -299,85 +290,78 @@ func authenticateRequestHandler(walletExt *walletextension.WalletExtension, user var reqJSONMap map[string]string err = json.Unmarshal(body, &reqJSONMap) if err != nil { - userConn.HandleError("Internal error") - walletExt.Logger().Error(fmt.Errorf("error unmarshaling request to authentcate: %w", err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("could not unmarshal address request - %w", err)) return } // get signature from the request and remove leading two bytes (0x) signature, err := hex.DecodeString(reqJSONMap[common.JSONKeySignature][2:]) if err != nil { - userConn.HandleError("Error: unable to decode signature") - walletExt.Logger().Error(fmt.Errorf("could not find or decode signature from client to hex: %w", err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("unable to decode signature - %w", err)) return } // get message from the request message, ok := reqJSONMap[common.JSONKeyMessage] if !ok || message == "" { - userConn.HandleError("Error: unable to read message field from the request") - walletExt.Logger().Error(fmt.Errorf("could not find message in the request: %w", err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("unable to read message field from the request")) return } // read userID from query params - hexUserID, err := getUserID(userConn, 2) + hexUserID, err := getUserID(conn, 2) if err != nil { - userConn.HandleError("Malformed query: 'u' required - representing userID") - walletExt.Logger().Error(fmt.Errorf("user not found in the query params: %w", err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("malformed query: 'u' required - representing userID - %w", err)) return } // check signature and add address and signature for that user err = walletExt.AddAddressToUser(hexUserID, message, signature) if err != nil { - userConn.HandleError("Internal error") - walletExt.Logger().Error(fmt.Errorf("error adding address to user with message: %s, %w", message, err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("internal error")) + walletExt.Logger().Error("error adding address to user with message", "message", message, log.ErrKey, err) return } - err = userConn.WriteResponse([]byte(common.SuccessMsg)) + err = conn.WriteResponse([]byte(common.SuccessMsg)) if err != nil { - walletExt.Logger().Error(fmt.Errorf("error writing success response, %w", err).Error()) - return + walletExt.Logger().Error("error writing success response", log.ErrKey, err) } } // This function handles request to /query endpoint. // In the query parameters address and userID are required. We check if provided address is registered for given userID // and return true/false in json response -func queryRequestHandler(walletExt *walletextension.WalletExtension, userConn userconn.UserConn) { +func queryRequestHandler(walletExt *walletextension.WalletExtension, conn userconn.UserConn) { // read the request - _, err := userConn.ReadRequest() + _, err := conn.ReadRequest() if err != nil { - userConn.HandleError("Error: bad request") - walletExt.Logger().Error(fmt.Errorf("error reading request: %w", err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("error reading request: %w", err)) return } - hexUserID, err := getUserID(userConn, 2) + hexUserID, err := getUserID(conn, 2) if err != nil { - userConn.HandleError("user ('u') not found in query parameters") - walletExt.Logger().Error(fmt.Errorf("user not found in the query params: %w", err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("user ('u') not found in query parameters")) + walletExt.Logger().Error("user not found in the query params", log.ErrKey, err) return } - address, err := getQueryParameter(userConn.ReadRequestParams(), common.AddressQueryParameter) + address, err := getQueryParameter(conn.ReadRequestParams(), common.AddressQueryParameter) if err != nil { - userConn.HandleError("address ('a') not found in query parameters") - walletExt.Logger().Error(fmt.Errorf("address not found in the query params: %w", err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("address ('a') not found in query parameters")) + walletExt.Logger().Error("address ('a') not found in query parameters", log.ErrKey, err) return } // check if address length is correct if len(address) != common.EthereumAddressLen { - userConn.HandleError(fmt.Sprintf("provided address length is %d, expected: %d", len(address), common.EthereumAddressLen)) - walletExt.Logger().Error(fmt.Errorf(fmt.Sprintf("provided address length is %d, expected: %d", len(address), common.EthereumAddressLen)).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("provided address length is %d, expected: %d", len(address), common.EthereumAddressLen)) return } // check if this account is registered with given user found, err := walletExt.UserHasAccount(hexUserID, address) if err != nil { - userConn.HandleError("Internal error") - walletExt.Logger().Error(fmt.Errorf("error during checking if account exists for user %s: %w", hexUserID, err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("internal error")) + walletExt.Logger().Error("error during checking if account exists for user", "hexUserID", hexUserID, log.ErrKey, err) } // create and write the response @@ -387,61 +371,59 @@ func queryRequestHandler(walletExt *walletextension.WalletExtension, userConn us msg, err := json.Marshal(res) if err != nil { - userConn.HandleError("Internal error") - walletExt.Logger().Error(fmt.Errorf("error marshalling: %w", err).Error()) + handleError(conn, walletExt.Logger(), err) return } - err = userConn.WriteResponse(msg) + err = conn.WriteResponse(msg) if err != nil { - walletExt.Logger().Error(fmt.Errorf("error writing success response, %w", err).Error()) - return + walletExt.Logger().Error("error writing success response", log.ErrKey, err) } } // This function handles request to /revoke endpoint. // It requires userID as query parameter and deletes given user and all associated viewing keys -func revokeRequestHandler(walletExt *walletextension.WalletExtension, userConn userconn.UserConn) { +func revokeRequestHandler(walletExt *walletextension.WalletExtension, conn userconn.UserConn) { // read the request - _, err := userConn.ReadRequest() + _, err := conn.ReadRequest() if err != nil { - walletExt.Logger().Error(fmt.Errorf("error reading request: %w", err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("error reading request: %w", err)) return } - hexUserID, err := getUserID(userConn, 2) + hexUserID, err := getUserID(conn, 2) if err != nil { - userConn.HandleError("user ('u') not found in query parameters") - walletExt.Logger().Error(fmt.Errorf("user not found in the query params: %w", err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("user ('u') not found in query parameters")) + walletExt.Logger().Error("user not found in the query params", log.ErrKey, err) return } - // delete user and accounts associated with it from the databse + // delete user and accounts associated with it from the database err = walletExt.DeleteUser(hexUserID) if err != nil { - userConn.HandleError("Internal error") - walletExt.Logger().Error(fmt.Errorf("unable to delete user %s: %w", hexUserID, err).Error()) + handleError(conn, walletExt.Logger(), fmt.Errorf("internal error")) + walletExt.Logger().Error("unable to delete user", "hexUserID", hexUserID, log.ErrKey, err) return } - err = userConn.WriteResponse([]byte(common.SuccessMsg)) + err = conn.WriteResponse([]byte(common.SuccessMsg)) if err != nil { - walletExt.Logger().Error(fmt.Errorf("error writing success response, %w", err).Error()) + walletExt.Logger().Error("error writing success response", log.ErrKey, err) } } // Handles request to /health endpoint. -func healthRequestHandler(walletExt *walletextension.WalletExtension, userConn userconn.UserConn) { +func healthRequestHandler(walletExt *walletextension.WalletExtension, conn userconn.UserConn) { // read the request - _, err := userConn.ReadRequest() + _, err := conn.ReadRequest() if err != nil { - walletExt.Logger().Error(fmt.Errorf("error reading request: %w", err).Error()) + walletExt.Logger().Error("error reading request", log.ErrKey, err) return } - err = userConn.WriteResponse([]byte(common.SuccessMsg)) + err = conn.WriteResponse([]byte(common.SuccessMsg)) if err != nil { - walletExt.Logger().Error(fmt.Errorf("error writing success response, %w", err).Error()) + walletExt.Logger().Error("error writing success response", log.ErrKey, err) } } @@ -450,12 +432,12 @@ func versionRequestHandler(walletExt *walletextension.WalletExtension, userConn // read the request _, err := userConn.ReadRequest() if err != nil { - walletExt.Logger().Error(fmt.Errorf("error reading request: %w", err).Error()) + walletExt.Logger().Error("error reading request", log.ErrKey, err) return } err = userConn.WriteResponse([]byte(walletExt.Version())) if err != nil { - walletExt.Logger().Error(fmt.Errorf("error writing success response, %w", err).Error()) + walletExt.Logger().Error("error writing success response", log.ErrKey, err) } } diff --git a/tools/walletextension/api/utils.go b/tools/walletextension/api/utils.go index 3bca5e921a..e954211e04 100644 --- a/tools/walletextension/api/utils.go +++ b/tools/walletextension/api/utils.go @@ -5,6 +5,8 @@ import ( "fmt" "strings" + gethlog "github.com/ethereum/go-ethereum/log" + "github.com/obscuronet/go-obscuro/tools/walletextension/accountmanager" "github.com/obscuronet/go-obscuro/tools/walletextension/common" "github.com/obscuronet/go-obscuro/tools/walletextension/userconn" @@ -81,3 +83,43 @@ func getUserID(conn userconn.UserConn, userIDPosition int) (string, error) { return userID, nil } + +func handleEthError(req *accountmanager.RPCRequest, conn userconn.UserConn, logger gethlog.Logger, err error) { + var method string + id := json.RawMessage("1") + if req != nil { + method = req.Method + id = req.ID + } + + errjson := &common.JSONError{ + Code: 0, + Message: err.Error(), + Data: nil, + } + + jsonRPRCError := common.JSONRPCMessage{ + Version: "2.0", + ID: id, + Method: method, + Params: nil, + Error: errjson, + Result: nil, + } + + errBytes, err := json.Marshal(jsonRPRCError) + if err != nil { + logger.Error("unable to marshal error - %w", err) + return + } + + if err = conn.WriteResponse(errBytes); err != nil { + logger.Error("unable to write response back - %w", err) + } +} + +func handleError(conn userconn.UserConn, logger gethlog.Logger, err error) { + if err = conn.WriteResponse([]byte(err.Error())); err != nil { + logger.Error("unable to write response back - %w", err) + } +} diff --git a/tools/walletextension/common/constants.go b/tools/walletextension/common/constants.go index 6c51c03202..97f0354de5 100644 --- a/tools/walletextension/common/constants.go +++ b/tools/walletextension/common/constants.go @@ -1,10 +1,7 @@ package common import ( - "fmt" "time" - - "github.com/obscuronet/go-obscuro/go/rpc" ) const ( @@ -53,7 +50,4 @@ const ( PathVersion = "/version/" ) -var ( - ErrSubscribeFailHTTP = fmt.Sprintf("received an %s request but the connection does not support subscriptions", rpc.Subscribe) - ReaderHeadTimeout = 10 * time.Second -) +var ReaderHeadTimeout = 10 * time.Second diff --git a/tools/walletextension/common/json.go b/tools/walletextension/common/json.go new file mode 100644 index 0000000000..caaf38330d --- /dev/null +++ b/tools/walletextension/common/json.go @@ -0,0 +1,20 @@ +package common + +import "encoding/json" + +// JSONRPCMessage value of this type can a JSON-RPC request, notification, successful response or +// error response. Which one it is depends on the fields. +type JSONRPCMessage struct { + Version string `json:"jsonrpc,omitempty"` + ID json.RawMessage `json:"id,omitempty"` + Method string `json:"method,omitempty"` + Params json.RawMessage `json:"params,omitempty"` + Error *JSONError `json:"error,omitempty"` + Result json.RawMessage `json:"result,omitempty"` +} + +type JSONError struct { + Code int `json:"code"` + Message string `json:"message"` + Data interface{} `json:"data,omitempty"` +} diff --git a/tools/walletextension/userconn/user_conn.go b/tools/walletextension/userconn/user_conn.go index e2d2790883..b600a54b39 100644 --- a/tools/walletextension/userconn/user_conn.go +++ b/tools/walletextension/userconn/user_conn.go @@ -1,24 +1,17 @@ package userconn import ( - "encoding/json" "fmt" "io" "net/http" "net/url" "strings" - gethlog "github.com/ethereum/go-ethereum/log" - - "github.com/obscuronet/go-obscuro/tools/walletextension/common" - "github.com/obscuronet/go-obscuro/go/common/log" - "github.com/gorilla/websocket" -) + gethlog "github.com/ethereum/go-ethereum/log" -const ( - httpCodeErr = 500 + "github.com/gorilla/websocket" ) var upgrader = websocket.Upgrader{} // Used to upgrade connections to websocket connections. @@ -28,7 +21,6 @@ type UserConn interface { ReadRequest() ([]byte, error) ReadRequestParams() map[string]string WriteResponse(msg []byte) error - HandleError(msg string) SupportsSubscriptions() bool IsClosed() bool GetHTTPRequest() *http.Request @@ -58,8 +50,8 @@ func NewUserConnWS(resp http.ResponseWriter, req *http.Request, logger gethlog.L conn, err := upgrader.Upgrade(resp, req, nil) if err != nil { err = fmt.Errorf("unable to upgrade to websocket connection - %w", err) + _, _ = resp.Write([]byte(err.Error())) logger.Error("unable to upgrade to websocket connection", log.ErrKey, err) - httpLogAndSendErr(resp, err.Error()) return nil, err } @@ -73,9 +65,7 @@ func NewUserConnWS(resp http.ResponseWriter, req *http.Request, logger gethlog.L func (h *userConnHTTP) ReadRequest() ([]byte, error) { body, err := io.ReadAll(h.req.Body) if err != nil { - wrappedErr := fmt.Errorf("could not read request body: %w", err) - h.HandleError(wrappedErr.Error()) - return nil, wrappedErr + return nil, fmt.Errorf("could not read request body: %w", err) } return body, nil } @@ -83,18 +73,11 @@ func (h *userConnHTTP) ReadRequest() ([]byte, error) { func (h *userConnHTTP) WriteResponse(msg []byte) error { _, err := h.resp.Write(msg) if err != nil { - wrappedErr := fmt.Errorf("could not write response: %w", err) - h.HandleError(wrappedErr.Error()) - return wrappedErr + return fmt.Errorf("could not write response: %w", err) } return nil } -func (h *userConnHTTP) HandleError(msg string) { - h.logger.Error(fmt.Sprintf("Handling HTTP user error - %s", msg)) - httpLogAndSendErr(h.resp, msg) -} - func (h *userConnHTTP) SupportsSubscriptions() bool { return false } @@ -117,9 +100,7 @@ func (w *userConnWS) ReadRequest() ([]byte, error) { if websocket.IsCloseError(err) { w.isClosed = true } - wrappedErr := fmt.Errorf("could not read request: %w", err) - w.HandleError(wrappedErr.Error()) - return nil, wrappedErr + return nil, fmt.Errorf("could not read request: %w", err) } return msg, nil } @@ -127,38 +108,14 @@ func (w *userConnWS) ReadRequest() ([]byte, error) { func (w *userConnWS) WriteResponse(msg []byte) error { err := w.conn.WriteMessage(websocket.TextMessage, msg) if err != nil { - if websocket.IsCloseError(err) { + if websocket.IsCloseError(err) || strings.Contains(string(msg), "EOF") { w.isClosed = true } - wrappedErr := fmt.Errorf("could not write response: %w", err) - w.HandleError(wrappedErr.Error()) - return wrappedErr + return fmt.Errorf("could not write response: %w", err) } return nil } -// HandleError logs and prints the error, and writes it to the websocket as a JSON object with a single key, "error". -func (w *userConnWS) HandleError(msg string) { - w.logger.Error(fmt.Sprintf("Handling WS user error - %s", msg)) - - errMsg, err := json.Marshal(map[string]interface{}{ - common.JSONKeyErr: msg, - }) - if err != nil { - w.logger.Error("could not marshal websocket error message to JSON", log.ErrKey, err) - return - } - - err = w.conn.WriteMessage(websocket.TextMessage, errMsg) - if err != nil { - if websocket.IsCloseError(err) || strings.Contains(msg, "EOF") { - w.isClosed = true - } - w.logger.Error("could not write error message to websocket", log.ErrKey, err) - return - } -} - func (w *userConnWS) SupportsSubscriptions() bool { return true } @@ -175,11 +132,6 @@ func (w *userConnWS) GetHTTPRequest() *http.Request { return w.req } -// Logs the error, prints it to the console, and returns the error over HTTP. -func httpLogAndSendErr(resp http.ResponseWriter, msg string) { - http.Error(resp, msg, httpCodeErr) -} - func getQueryParams(query url.Values) map[string]string { params := make(map[string]string) queryParams := query diff --git a/tools/walletextension/wallet_extension.go b/tools/walletextension/wallet_extension.go index 4b219207ef..7eae24b59a 100644 --- a/tools/walletextension/wallet_extension.go +++ b/tools/walletextension/wallet_extension.go @@ -95,19 +95,21 @@ func (w *WalletExtension) ProxyEthRequest(request *accountmanager.RPCRequest, co } err = selectedAccountManager.ProxyRequest(request, &rpcResp, conn) + if err != nil { + if errors.Is(err, rpc.ErrNilResponse) { + // if err was for a nil response then we will return an RPC result of null to the caller (this is a valid "not-found" response for some methods) + response[common.JSONKeyResult] = nil + return response, nil + } + return nil, err + } - if err != nil && !errors.Is(err, rpc.ErrNilResponse) { - response = common.CraftErrorResponse(err) - } else if errors.Is(err, rpc.ErrNilResponse) { - // if err was for a nil response then we will return an RPC result of null to the caller (this is a valid "not-found" response for some methods) - response[common.JSONKeyResult] = nil - } else { - response[common.JSONKeyResult] = rpcResp + response[common.JSONKeyResult] = rpcResp + + // todo (@ziga) - fix this upstream on the decode + // https://github.com/ethereum/EIPs/blob/master/EIPS/eip-658.md + adjustStateRoot(rpcResp, response) - // todo (@ziga) - fix this upstream on the decode - // https://github.com/ethereum/EIPs/blob/master/EIPS/eip-658.md - adjustStateRoot(rpcResp, response) - } return response, nil } From 288b591272687114f39245ce76906100a0887666 Mon Sep 17 00:00:00 2001 From: otherview Date: Fri, 13 Oct 2023 15:25:36 +0100 Subject: [PATCH 2/4] code rabbit findings --- tools/walletextension/api/utils.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/walletextension/api/utils.go b/tools/walletextension/api/utils.go index e954211e04..1bfd1427c0 100644 --- a/tools/walletextension/api/utils.go +++ b/tools/walletextension/api/utils.go @@ -3,6 +3,7 @@ package api import ( "encoding/json" "fmt" + "github.com/obscuronet/go-obscuro/go/common/log" "strings" gethlog "github.com/ethereum/go-ethereum/log" @@ -109,17 +110,17 @@ func handleEthError(req *accountmanager.RPCRequest, conn userconn.UserConn, logg errBytes, err := json.Marshal(jsonRPRCError) if err != nil { - logger.Error("unable to marshal error - %w", err) + logger.Error("unable to marshal error - %w", log.ErrKey, err) return } if err = conn.WriteResponse(errBytes); err != nil { - logger.Error("unable to write response back - %w", err) + logger.Error("unable to write response back - %w", log.ErrKey, err) } } func handleError(conn userconn.UserConn, logger gethlog.Logger, err error) { if err = conn.WriteResponse([]byte(err.Error())); err != nil { - logger.Error("unable to write response back - %w", err) + logger.Error("unable to write response back", log.ErrKey, err) } } From 723f1476b634bc8110329aeb9db5c910d71c04d4 Mon Sep 17 00:00:00 2001 From: otherview Date: Fri, 13 Oct 2023 15:30:25 +0100 Subject: [PATCH 3/4] lint --- tools/walletextension/api/utils.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/walletextension/api/utils.go b/tools/walletextension/api/utils.go index 1bfd1427c0..3f9bb468ae 100644 --- a/tools/walletextension/api/utils.go +++ b/tools/walletextension/api/utils.go @@ -3,9 +3,10 @@ package api import ( "encoding/json" "fmt" - "github.com/obscuronet/go-obscuro/go/common/log" "strings" + "github.com/obscuronet/go-obscuro/go/common/log" + gethlog "github.com/ethereum/go-ethereum/log" "github.com/obscuronet/go-obscuro/tools/walletextension/accountmanager" From 6fc2c4d302a8051212393460a286729149886828 Mon Sep 17 00:00:00 2001 From: otherview Date: Fri, 13 Oct 2023 15:49:35 +0100 Subject: [PATCH 4/4] fixing test --- tools/walletextension/test/wallet_extension_test.go | 7 +++---- tools/walletextension/wallet_extension.go | 2 -- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/tools/walletextension/test/wallet_extension_test.go b/tools/walletextension/test/wallet_extension_test.go index 7c1a92ae08..d51974d101 100644 --- a/tools/walletextension/test/wallet_extension_test.go +++ b/tools/walletextension/test/wallet_extension_test.go @@ -13,7 +13,6 @@ import ( "github.com/obscuronet/go-obscuro/go/common" "github.com/obscuronet/go-obscuro/go/rpc" "github.com/obscuronet/go-obscuro/integration" - "github.com/obscuronet/go-obscuro/tools/walletextension" "github.com/obscuronet/go-obscuro/tools/walletextension/accountmanager" "github.com/stretchr/testify/assert" @@ -150,9 +149,9 @@ func canInvokeSensitiveMethodsAfterSubmittingMultipleViewingKeys(t *testing.T, t func cannotSubscribeOverHTTP(t *testing.T, testHelper *testHelper) { respBody := makeHTTPEthJSONReq(testHelper.walletHTTPPort, rpc.Subscribe, []interface{}{rpc.SubscriptionTypeLogs}) - fmt.Println(respBody) - if string(respBody) != walletextension.ErrSubscribeFailHTTP+"\n" { - t.Fatalf("expected response of '%s', got '%s'", walletextension.ErrSubscribeFailHTTP, string(respBody)) + + if string(respBody) != "received an eth_subscribe request but the connection does not support subscriptions" { + t.Fatalf("unexpected response %s", string(respBody)) } } diff --git a/tools/walletextension/wallet_extension.go b/tools/walletextension/wallet_extension.go index 7eae24b59a..8650798736 100644 --- a/tools/walletextension/wallet_extension.go +++ b/tools/walletextension/wallet_extension.go @@ -27,8 +27,6 @@ import ( gethlog "github.com/ethereum/go-ethereum/log" ) -var ErrSubscribeFailHTTP = fmt.Sprintf("received an %s request but the connection does not support subscriptions", rpc.Subscribe) - // WalletExtension handles the management of viewing keys and the forwarding of Ethereum JSON-RPC requests. type WalletExtension struct { hostAddr string // The address on which the Obscuro host can be reached.