Improve gRPC error handling

This change removes non-idiomatic gRPC *Reply proto message fields.
The client should not receive success/fail values from server methods
with a void return type, nor an optional error_message from any server
method.  This change improves error handling by wrapping an appropriate
gRPC Status with a meaningful error description in a StatusRuntimeException,
and placing it in the server's response StreamObserver.  User error
messages are mapped to general purpose gRPC Status codes in a new ApiStatus
enum class.  (Maybe ApiStatus should be renamed to CoreApiStatus.)

cli/src/main/java/bisq/cli/CliMain.java

@@ -152,21 +152,13 @@ public class CliMain {
                     var stub = GetBalanceGrpc.newBlockingStub(channel).withCallCredentials(credentials);
                     var request = GetBalanceRequest.newBuilder().build();
                     var reply = stub.getBalance(request);
-                    if (reply.getBalance() == -1) {
-                        err.println(reply.getErrorMessage());
-                        exit(EXIT_FAILURE);
-                    }
                     out.println(formatBalance(reply.getBalance()));
                     exit(EXIT_SUCCESS);
                 }
                 case lockwallet: {
                     var stub = LockWalletGrpc.newBlockingStub(channel).withCallCredentials(credentials);
                     var request = LockWalletRequest.newBuilder().build();
-                    var reply = stub.lockWallet(request);
+                    stub.lockWallet(request);
-                    if (!reply.getSuccess()) {
-                        err.println(reply.getErrorMessage());
-                        exit(EXIT_FAILURE);
-                    }
                     out.println("wallet locked");
                     exit(EXIT_SUCCESS);
                 }
@@ -190,11 +182,7 @@ public class CliMain {
                     var request = UnlockWalletRequest.newBuilder()
                             .setPassword(nonOptionArgs.get(1))
                             .setTimeout(timeout).build();
-                    var reply = stub.unlockWallet(request);
+                    stub.unlockWallet(request);
-                    if (!reply.getSuccess()) {
-                        err.println(reply.getErrorMessage());
-                        exit(EXIT_FAILURE);
-                    }
                     out.println("wallet unlocked");
                     exit(EXIT_SUCCESS);
                 }
@@ -205,11 +193,7 @@ public class CliMain {
                     }
                     var stub = RemoveWalletPasswordGrpc.newBlockingStub(channel).withCallCredentials(credentials);
                     var request = RemoveWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).build();
-                    var reply = stub.removeWalletPassword(request);
+                    stub.removeWalletPassword(request);
-                    if (!reply.getSuccess()) {
-                        err.println(reply.getErrorMessage());
-                        exit(EXIT_FAILURE);
-                    }
                     out.println("wallet decrypted");
                     exit(EXIT_SUCCESS);
                 }
@@ -222,11 +206,7 @@ public class CliMain {
                     var request = (nonOptionArgs.size() == 3)
                             ? SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).setNewPassword(nonOptionArgs.get(2)).build()
                             : SetWalletPasswordRequest.newBuilder().setPassword(nonOptionArgs.get(1)).build();
-                    var reply = stub.setWalletPassword(request);
+                    stub.setWalletPassword(request);
-                    if (!reply.getSuccess()) {
-                        err.println(reply.getErrorMessage());
-                        exit(EXIT_FAILURE);
-                    }
                     out.println("wallet encrypted" + (nonOptionArgs.size() == 2 ? "" : " with new password"));
                     exit(EXIT_SUCCESS);
                 }
@@ -236,11 +216,17 @@ public class CliMain {
                 }
             }
         } catch (StatusRuntimeException ex) {
-            // This exception is thrown if the client-provided password credentials do not
+            // The actual server error message is in a nested exception and we clean it
-            // match the value set on the server. The actual error message is in a nested
+            // up a bit to make it more presentable.
-            // exception and we clean it up a bit to make it more presentable.
             Throwable t = ex.getCause() == null ? ex : ex.getCause();
-            err.println("Error: " + t.getMessage().replace("UNAUTHENTICATED: ", ""));
+            String message = t.getMessage()
+                    .replace("FAILED_PRECONDITION: ", "")
+                    .replace("INTERNAL: ", "")
+                    .replace("INVALID_ARGUMENT: ", "")
+                    .replace("UNAUTHENTICATED: ", "")
+                    .replace("UNAVAILABLE: ", "")
+                    .replace("UNKNOWN: ", "");
+            err.println("Error: " + message);
             exit(EXIT_FAILURE);
         }
     }

core/src/main/java/bisq/core/grpc/ApiStatus.java

@@ -0,0 +1,76 @@
+/*
+ * This file is part of Bisq.
+ *
+ * Bisq is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or (at
+ * your option) any later version.
+ *
+ * Bisq is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
+ * License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with Bisq. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package bisq.core.grpc;
+
+import io.grpc.Status;
+
+/**
+ * Maps meaningful CoreApi error messages to more general purpose gRPC error Status codes.
+ * This keeps gRPC api out of CoreApi, and ensures the correct gRPC Status is sent to the
+ * client.
+ *
+ * @see <a href="https://github.com/grpc/grpc/blob/master/doc/statuscodes.md">gRPC Status Codes</a>
+ */
+enum ApiStatus {
+
+    INCORRECT_OLD_WALLET_PASSWORD(Status.INVALID_ARGUMENT, "incorrect old password"),
+    INCORRECT_WALLET_PASSWORD(Status.INVALID_ARGUMENT, "incorrect password"),
+
+    INTERNAL(Status.INTERNAL, "internal server error"),
+
+    OK(Status.OK, null),
+
+    UNKNOWN(Status.UNKNOWN, "unknown"),
+
+    WALLET_ALREADY_LOCKED(Status.FAILED_PRECONDITION, "wallet is already locked"),
+
+    WALLET_ENCRYPTER_NOT_AVAILABLE(Status.FAILED_PRECONDITION, "wallet encrypter is not available"),
+
+    WALLET_IS_ENCRYPTED(Status.FAILED_PRECONDITION, "wallet is encrypted with a password"),
+    WALLET_IS_ENCRYPTED_WITH_UNLOCK_INSTRUCTION(Status.FAILED_PRECONDITION,
+            "wallet is encrypted with a password; unlock it with the 'unlock \"password\" timeout' command"),
+
+    WALLET_NOT_ENCRYPTED(Status.FAILED_PRECONDITION, "wallet is not encrypted with a password"),
+    WALLET_NOT_AVAILABLE(Status.UNAVAILABLE, "wallet is not available");
+
+
+    private final Status grpcStatus;
+    private final String description;
+
+    ApiStatus(Status grpcStatus, String description) {
+        this.grpcStatus = grpcStatus;
+        this.description = description;
+    }
+
+    Status getGrpcStatus() {
+        return this.grpcStatus;
+    }
+
+    String getDescription() {
+        return this.description;
+    }
+
+    @Override
+    public String toString() {
+        return "ApiStatus{" +
+                "grpcStatus=" + grpcStatus +
+                ", description='" + description + '\'' +
+                '}';
+    }
+}
+

core/src/main/java/bisq/core/grpc/CoreApi.java

@@ -51,6 +51,7 @@ import lombok.extern.slf4j.Slf4j;
 
 import javax.annotation.Nullable;
 
+import static bisq.core.grpc.ApiStatus.*;
 import static java.util.concurrent.TimeUnit.SECONDS;
 
 /**
@@ -91,18 +92,21 @@ public class CoreApi {
         return Version.VERSION;
     }
 
-    public Tuple2<Long, String> getAvailableBalance() {
+    public Tuple2<Long, ApiStatus> getAvailableBalance() {
         if (!walletsManager.areWalletsAvailable())
-            return new Tuple2<>(-1L, "Error: wallet is not available");
+            return new Tuple2<>(-1L, WALLET_NOT_AVAILABLE);
 
         if (walletsManager.areWalletsEncrypted())
-            return new Tuple2<>(-1L, "Error: wallet is encrypted; unlock it with the 'unlock \"password\" timeout' command");
+            return new Tuple2<>(-1L, WALLET_IS_ENCRYPTED_WITH_UNLOCK_INSTRUCTION);
 
         try {
             long balance = balances.getAvailableBalance().get().getValue();
-            return new Tuple2<>(balance, "");
+            return new Tuple2<>(balance, OK);
         } catch (Throwable t) {
-            return new Tuple2<>(-1L, "Error: " + t.getLocalizedMessage());
+            // TODO Derive new ApiStatus codes from server stack traces.
+            t.printStackTrace();
+            // TODO Fix bug causing NPE thrown by getAvailableBalance().
+            return new Tuple2<>(-1L, INTERNAL);
         }
     }
 
@@ -183,74 +187,78 @@ public class CoreApi {
 
     // Provided for automated wallet protection method testing, despite the
     // security risks exposed by providing users the ability to decrypt their wallets.
-    public Tuple2<Boolean, String> removeWalletPassword(String password) {
+    public Tuple2<Boolean, ApiStatus> removeWalletPassword(String password) {
         if (!walletsManager.areWalletsAvailable())
-            return new Tuple2<>(false, "Error: wallet is not available");
+            return new Tuple2<>(false, WALLET_NOT_AVAILABLE);
 
         if (!walletsManager.areWalletsEncrypted())
-            return new Tuple2<>(false, "Error: wallet is not encrypted with a password");
+            return new Tuple2<>(false, WALLET_NOT_ENCRYPTED);
 
         KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt();
         if (keyCrypterScrypt == null)
-            return new Tuple2<>(false, "Error: wallet encrypter is not available");
+            return new Tuple2<>(false, WALLET_ENCRYPTER_NOT_AVAILABLE);
 
         KeyParameter aesKey = keyCrypterScrypt.deriveKey(password);
         if (!walletsManager.checkAESKey(aesKey))
-            return new Tuple2<>(false, "Error: incorrect password");
+            return new Tuple2<>(false, INCORRECT_WALLET_PASSWORD);
 
         walletsManager.decryptWallets(aesKey);
-        return new Tuple2<>(true, "");
+        return new Tuple2<>(true, OK);
     }
 
-    public Tuple2<Boolean, String> setWalletPassword(String password, String newPassword) {
+    public Tuple2<Boolean, ApiStatus> setWalletPassword(String password, String newPassword) {
         try {
             if (!walletsManager.areWalletsAvailable())
-                return new Tuple2<>(false, "Error: wallet is not available");
+                return new Tuple2<>(false, WALLET_NOT_AVAILABLE);
 
             KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt();
             if (keyCrypterScrypt == null)
-                return new Tuple2<>(false, "Error: wallet encrypter is not available");
+                return new Tuple2<>(false, WALLET_ENCRYPTER_NOT_AVAILABLE);
 
             if (newPassword != null && !newPassword.isEmpty()) {
-                // todo validate new password
+                // TODO Validate new password before replacing old password.
                 if (!walletsManager.areWalletsEncrypted())
-                    return new Tuple2<>(false, "Error: wallet is not encrypted with a password");
+                    return new Tuple2<>(false, WALLET_NOT_ENCRYPTED);
 
                 KeyParameter aesKey = keyCrypterScrypt.deriveKey(password);
                 if (!walletsManager.checkAESKey(aesKey))
-                    return new Tuple2<>(false, "Error: incorrect old password");
+                    return new Tuple2<>(false, INCORRECT_OLD_WALLET_PASSWORD);
 
                 walletsManager.decryptWallets(aesKey);
                 aesKey = keyCrypterScrypt.deriveKey(newPassword);
                 walletsManager.encryptWallets(keyCrypterScrypt, aesKey);
-                return new Tuple2<>(true, "");
+                return new Tuple2<>(true, OK);
             }
 
             if (walletsManager.areWalletsEncrypted())
-                return new Tuple2<>(false, "Error: wallet is already encrypted");
+                return new Tuple2<>(false, WALLET_IS_ENCRYPTED);
+
+            // TODO Validate new password.
             KeyParameter aesKey = keyCrypterScrypt.deriveKey(password);
             walletsManager.encryptWallets(keyCrypterScrypt, aesKey);
-            return new Tuple2<>(true, "");
+            return new Tuple2<>(true, OK);
         } catch (Throwable t) {
-            return new Tuple2<>(false, "Error: " + t.getLocalizedMessage());
+            // TODO Derive new ApiStatus codes from server stack traces.
+            t.printStackTrace();
+            return new Tuple2<>(false, INTERNAL);
         }
     }
 
-    public Tuple2<Boolean, String> lockWallet() {
+    public Tuple2<Boolean, ApiStatus> lockWallet() {
         if (tempLockWalletPassword != null) {
-            Tuple2<Boolean, String> encrypted = setWalletPassword(tempLockWalletPassword, null);
+            Tuple2<Boolean, ApiStatus> encrypted = setWalletPassword(tempLockWalletPassword, null);
             tempLockWalletPassword = null;
-            if (!encrypted.first)
+            if (!encrypted.second.equals(OK))
                 return encrypted;
 
-            return new Tuple2<>(true, "");
+            return new Tuple2<>(true, OK);
         }
-        return new Tuple2<>(false, "Error: wallet is already locked");
+        return new Tuple2<>(false, WALLET_ALREADY_LOCKED);
     }
 
-    public Tuple2<Boolean, String> unlockWallet(String password, long timeout) {
+    public Tuple2<Boolean, ApiStatus> unlockWallet(String password, long timeout) {
-        Tuple2<Boolean, String> decrypted = removeWalletPassword(password);
+        Tuple2<Boolean, ApiStatus> decrypted = removeWalletPassword(password);
-        if (!decrypted.first)
+        if (!decrypted.second.equals(OK))
             return decrypted;
 
         TimerTask timerTask = new TimerTask() {
@@ -267,6 +275,6 @@ public class CoreApi {
         // Cache wallet password for timeout (secs), in case
         // user wants to lock the wallet for timeout expires.
         tempLockWalletPassword = password;
-        return new Tuple2<>(true, "");
+        return new Tuple2<>(true, OK);
     }
 }

core/src/main/java/bisq/core/grpc/GrpcServer.java

@@ -56,6 +56,7 @@ import bisq.proto.grpc.UnlockWalletReply;
 import bisq.proto.grpc.UnlockWalletRequest;
 
 import io.grpc.ServerBuilder;
+import io.grpc.StatusRuntimeException;
 import io.grpc.stub.StreamObserver;
 
 import java.io.IOException;
@@ -114,7 +115,13 @@ public class GrpcServer {
         @Override
         public void getBalance(GetBalanceRequest req, StreamObserver<GetBalanceReply> responseObserver) {
             var result = coreApi.getAvailableBalance();
-            var reply = GetBalanceReply.newBuilder().setBalance(result.first).setErrorMessage(result.second).build();
+            if (!result.second.equals(ApiStatus.OK)) {
+                StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus()
+                        .withDescription(result.second.getDescription()));
+                responseObserver.onError(ex);
+                throw ex;
+            }
+            var reply = GetBalanceReply.newBuilder().setBalance(result.first).build();
             responseObserver.onNext(reply);
             responseObserver.onCompleted();
         }
@@ -191,8 +198,13 @@ public class GrpcServer {
         public void removeWalletPassword(RemoveWalletPasswordRequest req,
                                          StreamObserver<RemoveWalletPasswordReply> responseObserver) {
             var result = coreApi.removeWalletPassword(req.getPassword());
-            var reply = RemoveWalletPasswordReply.newBuilder()
+            if (!result.second.equals(ApiStatus.OK)) {
-                    .setSuccess(result.first).setErrorMessage(result.second).build();
+                StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus()
+                        .withDescription(result.second.getDescription()));
+                responseObserver.onError(ex);
+                throw ex;
+            }
+            var reply = RemoveWalletPasswordReply.newBuilder().build();
             responseObserver.onNext(reply);
             responseObserver.onCompleted();
         }
@@ -203,8 +215,13 @@ public class GrpcServer {
         public void setWalletPassword(SetWalletPasswordRequest req,
                                       StreamObserver<SetWalletPasswordReply> responseObserver) {
             var result = coreApi.setWalletPassword(req.getPassword(), req.getNewPassword());
-            var reply = SetWalletPasswordReply.newBuilder()
+            if (!result.second.equals(ApiStatus.OK)) {
-                    .setSuccess(result.first).setErrorMessage(result.second).build();
+                StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus()
+                        .withDescription(result.second.getDescription()));
+                responseObserver.onError(ex);
+                throw ex;
+            }
+            var reply = SetWalletPasswordReply.newBuilder().build();
             responseObserver.onNext(reply);
             responseObserver.onCompleted();
         }
@@ -215,8 +232,13 @@ public class GrpcServer {
         public void lockWallet(LockWalletRequest req,
                                StreamObserver<LockWalletReply> responseObserver) {
             var result = coreApi.lockWallet();
-            var reply = LockWalletReply.newBuilder()
+            if (!result.second.equals(ApiStatus.OK)) {
-                    .setSuccess(result.first).setErrorMessage(result.second).build();
+                StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus()
+                        .withDescription(result.second.getDescription()));
+                responseObserver.onError(ex);
+                throw ex;
+            }
+            var reply = LockWalletReply.newBuilder().build();
             responseObserver.onNext(reply);
             responseObserver.onCompleted();
         }
@@ -227,8 +249,13 @@ public class GrpcServer {
         public void unlockWallet(UnlockWalletRequest req,
                                  StreamObserver<UnlockWalletReply> responseObserver) {
             var result = coreApi.unlockWallet(req.getPassword(), req.getTimeout());
-            var reply = UnlockWalletReply.newBuilder()
+            if (!result.second.equals(ApiStatus.OK)) {
-                    .setSuccess(result.first).setErrorMessage(result.second).build();
+                StatusRuntimeException ex = new StatusRuntimeException(result.second.getGrpcStatus()
+                        .withDescription(result.second.getDescription()));
+                responseObserver.onError(ex);
+                throw ex;
+            }
+            var reply = UnlockWalletReply.newBuilder().build();
             responseObserver.onNext(reply);
             responseObserver.onCompleted();
         }

proto/src/main/proto/grpc.proto

@@ -53,7 +53,6 @@ message GetBalanceRequest {
 
 message GetBalanceReply {
     uint64 balance = 1;
-    string error_message = 2; // optional error message
 }
 
 ///////////////////////////////////////////////////////////////////////////////////////////
@@ -143,8 +142,6 @@ message RemoveWalletPasswordRequest {
 }
 
 message RemoveWalletPasswordReply {
-    bool success = 1;
-    string error_message = 2; // optional error message
 }
 
 ///////////////////////////////////////////////////////////////////////////////////////////
@@ -162,8 +159,6 @@ message SetWalletPasswordRequest {
 }
 
 message SetWalletPasswordReply {
-    bool success = 1;
-    string error_message = 2; // optional error message
 }
 
 ///////////////////////////////////////////////////////////////////////////////////////////
@@ -179,8 +174,6 @@ message LockWalletRequest {
 }
 
 message LockWalletReply {
-    bool success = 1;
-    string error_message = 2; // optional error message
 }
 
 ///////////////////////////////////////////////////////////////////////////////////////////
@@ -198,6 +191,4 @@ message UnlockWalletRequest {
 }
 
 message UnlockWalletReply {
-    bool success = 1;
-    string error_message = 2; // optional error message
 }