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

refactor(sdk): always send RPC requests via network and deeplink #1181

Merged
merged 4 commits into from
Dec 19, 2024

Conversation

abretonc7s
Copy link
Collaborator

@abretonc7s abretonc7s commented Dec 18, 2024

Description

This PR simplifies the RPC message handling strategy by sending requests through both network and deeplink channels simultaneously, allowing the wallet to process whichever arrives first. This approach eliminates edge cases and improves reliability while maintaining backward compatibility.

Technical Details

  • Previously, we conditionally sent RPC requests either via network or deeplink based on protocol availability
  • Now, we:
    1. Always send RPC requests via network for consistency and reliability
    2. Allow deeplink protocol to handle requests in parallel when available
    3. Let the wallet deduplicate and process only the first received request
    4. Remove conditional analytics sending since it's now handled uniformly

Migration

No migration steps required. This is a transparent change that maintains backward compatibility.

Related:

- Moved RPC method tracking logic to ensure it is executed before sending analytics.
- Enhanced logging for the RPC method tracker to include detailed information about the tracked methods and data.
- Removed redundant console warnings to clean up the code and improve readability.
@abretonc7s abretonc7s changed the title fix: analytics done event on deeplink protocol fix: ensure RPC tracking for direct deeplink interactions Dec 18, 2024
@abretonc7s abretonc7s marked this pull request as ready for review December 18, 2024 15:01
@abretonc7s abretonc7s requested a review from a team as a code owner December 18, 2024 15:01
Copy link

codecov bot commented Dec 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.13%. Comparing base (f9a9921) to head (ed6edbc).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1181      +/-   ##
==========================================
- Coverage   74.14%   74.13%   -0.02%     
==========================================
  Files         181      181              
  Lines        4305     4299       -6     
  Branches     1056     1053       -3     
==========================================
- Hits         3192     3187       -5     
+ Misses       1113     1112       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ecp4224
ecp4224 previously approved these changes Dec 18, 2024
• Remove conditional analytics sending
• Always send RPC requests via network
• Add debug logging for triggeredInstaller state
• Remove redundant RPC method tracking
@abretonc7s abretonc7s changed the title fix: ensure RPC tracking for direct deeplink interactions refactor(sdk): always send RPC requests via network and deeplink Dec 19, 2024
Copy link
Collaborator

@christopherferreira9 christopherferreira9 left a comment

Choose a reason for hiding this comment

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

LGTM

@abretonc7s abretonc7s merged commit 10be04e into main Dec 19, 2024
35 of 36 checks passed
@abretonc7s abretonc7s deleted the analyticsdone branch December 19, 2024 09:59
@christopherferreira9
Copy link
Collaborator

First request analytics are coming through after connecting.

Screen.Recording.2024-12-19.at.10.02.58.AM.mov

@christopherferreira9 christopherferreira9 mentioned this pull request Dec 19, 2024
3 tasks
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.

3 participants