metaplex-foundation / metaplex-android

Metaplex Mobile Android SDK
18 stars 12 forks source link

metaplex.nft.findAllByOwner blows up with async exception #79

Open clarkezone opened 1 year ago

clarkezone commented 1 year ago

Describe the bug

I'm attempting to convert https://github.com/clarkezone/NATIV to use the metaplex android APIs from hand coded json parsing as suggested by @creativedrewy. I've copied the basic code samples from this repo to attempt to enumerate NFTs for a public address but the findAllByOwner method is currently blowing up with an async exception.

To Reproduce

  1. git clone https://github.com/clarkezone/NATIV.git
  2. open solution in Android Studio
  3. open up SolanaNFT/src/main/java/com/creativedrewy/solananft/metaplex/MetaplexNftUseCase.kt
  4. Put a breakpoint on line 40: metaplex.nft.findAllByOwner(ownerPublicKey).apply {
  5. Launch android studio debugger and confirm breakpoint is hit
  6. Step over breakpoint

Expected behavior

Method call succeeds or else the onfailure block is hit

Actual behavior

Unhandled async exception. When launching without debug, app exits

Screenshots

If applicable, add screenshots to help explain your problem.

SDK Version & Context

Platform: Android Version 1.2.0 Additional context

Funkatronics commented 1 year ago

Hi @clarkezone I am not able to reproduce this issue on my end. Can you please add a minimal reproducible example?

Funkatronics commented 1 year ago

I've added a bug report template, would you mind reformatting/reopening the issue with this template? this is a great help to us and will expedite the fix for you

clarkezone commented 1 year ago

I will see what i can do today, and will also look at bug template. The repo in the initial bug report has the code / state that reproduces the problem BTW

clarkezone commented 1 year ago

OK I updated the bug above with a set of steps. LMK if you can repro based on those.

Funkatronics commented 1 year ago

Nice, really appreciate the detailed report. I am flying back from Lisbon to the states tonight but I will check this out in the next day or two and try to get a fix out!

clarkezone commented 1 year ago

Awesome thx, me too! Once you track down issue any debugging techniques that helped would be appreciated for tracking this kind of Async exception on android down in future to provide more targeted report and or PR with fix ;-)

Funkatronics commented 1 year ago

I'd have to look deeper into your application code but async exceptions like this are typically do to mismanagement of the error within an unconfined coroutine context. I see that your app is using the default dispatcher and RPC driver. Is there a reason why you are not using the RpcRequestClient as your RPC driver for the NftClient? It looks to me like it oculd easily be extended to implement the JsonRpcDriver interface. This is is probably not necessary but just trying to understand more about the app architecture here.

I am trying to create a minimal reproduceable example so I can determine if this is an error in the library error handling, or an error in your app code, but I am not finding anything. We do have some gross error wrapping in the Nft Client that I need to clean up, but so far in my testing the findAllByOwner method is handling errors properly and always returns a Result.Failure. I am also pretty sick right now with a head cold so not in my best mental state tbh 😅

In order to nail down exactly where the error is occurring, I would recommend unit testing the MetaplexNftUseCase class within the SolanaNFT module. Unit tests should always be the first line of defense, as trying to debug this kind of error in the app itself is really difficult. Some unit tests would help you find out where in the stack the error is occurring, which would then help me assist you further.

Also, have you tried using the latest beta release instead of 1.2.0? Not much has changed within the findAllByOwner method so I doubt this will fix it for you, but still best to test against the latest code IMO.

Funkatronics commented 1 year ago

Really do appreciate your report here tho, and I want to help you track down the error so we can improve the library and make it easier to use. I think adding some testing with flows on our end would be good so we can ensure everything works (like error handling) when using the client in a flow context.

clarkezone commented 1 year ago

I forked this project at Breakpoint based on a conversation with @creativedrewy who's one of the devs on Solana Mobile with the intent of replacing his hand rolled json parsing with the Metaplex SDK (again, his suggestion). I don't have a lot of experience with Android dev so can't speak to your questions easily about dispatcher choices, was intending to make the most minimally invasive change possible to his app, but if there is a need to wholesale change the dispatcher model I could look at that after some heavy consultation with docs :-)

clarkezone commented 1 year ago

BTW thanks for looking and trying to help out here, really appreciate it!

clarkezone commented 1 year ago

I found some quite good docs on coroutines and dispatchers, I'll see if i can make a unit test for that class as you suggest. Also, hope you feel better sorry about the head cold!