FireStack-Lab / LaksaJ

LaksaJ -- Zilliqa Blockchain Java Library
GNU General Public License v3.0
19 stars 9 forks source link

Cannot get error message on failure on HttpProvider.createTransaction() #13

Closed ByunghyunBang closed 5 years ago

ByunghyunBang commented 5 years ago

Cannot get error message such like "invalid CHAIN_ID" or "insufficient gas". When the function has failed, getResult() returns just null and I don't know what the error message was.

example #1 : {"error":{"code":-26,"data":null,"message":"CHAIN_ID incorrect"},"id":"1","jsonrpc":"2.0"}

example #2 : {"error":{"code":-26,"data":null,"message":"GasPrice 1 lower than minimum allowable 1000000000"},"id":"1","jsonrpc":"2.0"}

Please add codes to raise exception to get error messsage for each function. code example here : (similar to function HttpProvider.getTransaction())

    public Rep<CreateTxResult> createTransaction(TransactionPayload payload) throws IOException {
        Req req = Req.builder().id("1").jsonrpc("2.0").method("CreateTransaction").params(new Object[]{payload}).build();
        Response response = client.newCall(buildRequest(req)).execute();
        String resultString = Objects.requireNonNull(response.body()).string();
        Type type = new TypeToken<Rep<CreateTxResult>>() {
        }.getType();
        Rep<CreateTxResult> rep = gson.fromJson(resultString, type);
+        if (rep.getResult() == null) {
+            throw new IOException("get result error = " + resultString);
+        }
        return rep;
    }
renlulu commented 5 years ago

I see. I will sort all exceptions out, maybe a enum? And will raise them. Thank you for your feedback.

erickorbit commented 5 years ago

In additional comment, It had better make your separated Exception such like "ZilliqaAPIException" to seperate from just network problem. which throws when respons was successfully received but the result has error message.

I think you don't need to sort all exceptions message. It will be better raise exception with just message itself.

renlulu commented 5 years ago

Sounds good! Thank you for your suggestion!

renlulu commented 5 years ago

Hey guys, I just addressed this issue by making a ZilliqaAPIException and raise it inside class Provider, almost it's erickorbit 's idea, thank you!

By the way, because this library is already used by some organizations, so I cannot change all functions right away for some reasons. I will think of a good way. Hope your review and more suggestions!

erickorbit commented 5 years ago

It's great!

I looked your commit https://github.com/FireStack-Lab/LaksaJ/commit/45b844264eb83b09094b0de5cc6993a70a34cf69.

I have another idea to improve error handling. Users which use this sdk might want to get both error code and message. but your parseError() function only get message.

example. {"error":{"code":-26,"data":null,"message":"CHAIN_ID incorrect"},"id":"1","jsonrpc":"2.0"}

In this case, ZilliqaAPIException message would be "CHAIN_ID incorrect". In my oponion, the message would be better contain error code such like "[code:-26] CHAIN_ID incorrect".

Thank you for adopting my opinions.

renlulu commented 5 years ago

Sounds about right, but I think it will be better to add another field to hold error code?

erickorbit commented 5 years ago

OK, it also would be good.

erickorbit commented 5 years ago

When you release this code? I need this code for ZIL mainnet support until next week.

renlulu commented 5 years ago

I have released severl artificts to maven center repository . You can directly search laksaj.

compile group: 'org.firestack', name: 'laksaj', version: '0.4.5-RELEASE', ext: 'pom'

But I haven't release latest commit yet. I will do it right away, it will be 0.4.6-RELEASE