nblockchain / geewallet

geewallet is a non-custodial, minimalistic & pragmatist opensource crossplatform lightweight brainwallet to hold the most important cryptoassets in the same application with ease & peace of mind
MIT License
61 stars 37 forks source link

Backend: handle null response from Eth server #287

Closed webwarrior-ws closed 2 months ago

webwarrior-ws commented 2 months ago

Introduce WeirdNullResponseException type and raise it when response for balance request for Ethereum returns json with "result" field value of null. This way server will be marked as faulty instead of crashing the application.

Fixes https://github.com/nblockchain/geewallet/issues/282

webwarrior-ws commented 2 months ago

I've added null checks to other Eth requests. There are 2 places that already do null checks, but raise generic Exception: https://github.com/webwarrior-ws/geewallet/blob/88e1be44b9c98f17e84c9337d21eb91939bb1e9e/src/GWallet.Backend/Ether/EtherServer.fs#L504 https://github.com/webwarrior-ws/geewallet/blob/88e1be44b9c98f17e84c9337d21eb91939bb1e9e/src/GWallet.Backend/Ether/EtherServer.fs#L599

Should I also convert them to the new exception type?

knocte commented 2 months ago

Yes

knocte commented 2 months ago

Backend: add another Eth RPC error type

Backend: handle error 408 in Eth code

When you say "In Eth code" you are defining a scope. We already have conventions in the way we write git commit messages to specify what part of the code you are modifying: it is the scope, the first part of the commit message title, before the colon. The scope can be more specific than the one you wrote, thanks to the fact that we also allow subscopes. So, replace Backend: with Backend/Ether: to not need to write things like "In Eth code". Thanks to this, you won't need to many characters to specify where in the code you're changing stuff. And thanks to having less characters in the title, you can be more specific about what you're doing. For example, I'd prefer handle/retry instead of just handle, as it's more specific about what the code will really do after your commit. Last but not least, given that both the commits above are doing a very similar thing, let's make their commit titles as similar as possible.

knocte commented 2 months ago

Last but not least, given that both the commits above are doing a very similar thing, let's make their commit titles as similar as possible.

You ignored the sentence above.

BTW let's add an additional final commit that refactors some stuff: when reviewing this diff I saw a couple of null checks that looked weird:

1)

if foo <> null then...

2)

if Object.ReferenceEquals(null, foo) then
...

Let's replace both of those types (all occurrences) with isNull.

knocte commented 2 months ago

Let's rather make this PR target the stable branch.

webwarrior-ws commented 2 months ago

rebased on stable

knocte commented 2 months ago

Two last things to do here before I merge this: