Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose Deauthorize Requests/Event to React Native #505

Merged
merged 1 commit into from
Jul 22, 2023

Conversation

Funkatronics
Copy link
Contributor

No description provided.

Copy link
Contributor

@Michaelsulistio Michaelsulistio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! After these changes, there's a couple of TODOs left that we synced about offline.

  1. Move initializeScenario into useEffect to avoid redundant, expensive operations.
  2. On JS side, fix the redundant authorizationScope field inAuthorizationRequest.

@@ -76,9 +77,10 @@ internal object MobileWalletAdapterResponseSerializer : JsonContentPolymorphicSe
override fun selectDeserializer(element: JsonElement): DeserializationStrategy<out MobileWalletAdapterResponse> =
if ((element as? JsonObject)?.containsKey("failReason") == true) FailReasonTransformingSerializer
else if ((element as? JsonObject)?.containsKey("publicKey") == true) AuthorizeDappResponse.serializer()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we also include a __type in the response object, so instead of checking for unique fields we can just check a more readable __type field. Not a big deal/blocker though and probably not worth a whole refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I actually started to add that but ran into some issues on the JS side due to the use of types vs classes. I couldn't find a clean way to enforce the __type field so it was getting omitted if I did not explicitly add it to every response, which seemed like a highly error prone situation if we are relying on those types for deserialization. Obviously there has to be a better way to handle this but decided to leave it out for a separate PR.

@Funkatronics Funkatronics merged commit c8b24f0 into main Jul 22, 2023
@Funkatronics Funkatronics deleted the rn-walletlib--expose-deauthorize branch July 22, 2023 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants