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

feat: Change network status check feature in NetInfo, disable/enable mediator check and provide new method to check isInternetReachable #1390

Merged
merged 17 commits into from
Jan 22, 2025

Conversation

jian4on
Copy link
Contributor

@jian4on jian4on commented Jan 10, 2025

Summary of Changes

  • added Config Token to disable/enable mediator check
  • added Config Token to define internet reachability URLs, which are used as beacon for internet status testing
  • added new method in networkProvider to check isInternetReachable
  • implemented the new network check feature to NetInfo component

Screenshots, videos, or gifs

N/A

Breaking change guide

N/A

Related Issues

N/A

Pull Request Checklist

Tick all boxes below to demonstrate that you have completed the respective task. If the item does not apply to your this PR check it anyway to make it apparent that there's nothing to do.

  • All commits contain a DCO Signed-off-by line (we use the DCO GitHub app to enforce this)
  • If applicable, screenshots, gifs, or video are included for UI changes
  • If applicable, breaking changes are described above along with how to address them
  • Updated documentation as needed for changed code and new or modified features
  • Added sufficient tests so that overall code coverage is not reduced

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

Pro Tip 🤓

  • Read our contribution guide at least once; it will save you a few review cycles!
  • Your PR will likely not be reviewed until all the above boxes are checked and all automated checks have passed

@jian4on jian4on requested a review from a team as a code owner January 10, 2025 20:40
@jian4on
Copy link
Contributor Author

jian4on commented Jan 10, 2025

Will fix the __test, new provider spy failed in other feature testing suites

@jian4on
Copy link
Contributor Author

jian4on commented Jan 10, 2025

Native Build & Test / build-ios failed, it is the boost installing error happens in the new year. The reason is the host changed - "The boost download was hosted on JFrog until December 31, 2024 when the service ended. Therefore the problem appeared out of nowhere" on Jan 1, 2025".
see the discussing at Here

…isableFirewallCheck not assert. But skip mediator connecting when it is disabled and check isInternetReachable instead

Signed-off-by: Jian Wang <[email protected]>
…internet testing beacons, the networkProvider provides isInternetReachable API according to connecting to 'internetReachabilityUrls'

Signed-off-by: Jian Wang <[email protected]>
@jian4on jian4on changed the title feat: Changed network status check logic in NetInfo and add config to control firewall check feat: Change network status check feature in NetInfo, add config token to control firewall check and provide 'internetReachabilityUrls' for isInternetReachable check Jan 14, 2025
@jian4on jian4on changed the title feat: Change network status check feature in NetInfo, add config token to control firewall check and provide 'internetReachabilityUrls' for isInternetReachable check feat: Change network status check feature in NetInfo, disable/enable mediator check and provide new method to check isInternetReachable Jan 14, 2025
Signed-off-by: Jian Wang <[email protected]>
return isConnected
}

const assertNetworkReachable = async (): Promise<boolean> => {
// Ping internet check beacon endponits, set internetReachable to be true positive response from anyone
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use the netinfo and not custom code for the reachability check. We can pass custom URLs to the netinfo library if required. The reason for this is that Android has built in reachability checks that netinfo will use if available. The use of google is a fallback. On IOS the netinfo library does use the fallback for now. @jian4on

@jleach jleach marked this pull request as draft January 14, 2025 16:23
…, and back using built-in 'isInternetReachable', the custom beakons ping is commented

Signed-off-by: Jian Wang <[email protected]>
… not asserted and clear the Toast message once internetReachableCheck passed

Signed-off-by: Jian Wang <[email protected]>
@timbl-ont
Copy link
Contributor

Context for this PR:

When using the oid4vci flow, a mediator is not necessary. If a mediator is not present (or not needed for the wallet to function in a use case) then there needs to be another way to confirm Internet connectivity. The library react-native-netinfo is already used in the code base and is able to make use of IOS and Android functionality to test Internet reachability with lots of configurability. The other option was to create another .env variable with alternate sites to test that would be used by the existing code instead of the mediator. The assumption is that the native reachability functionality in the OS will scale better then a custom solution. Netinfo also has many small optimizations for corner cases and is well maintained.

There is still some overlap in functionality that could be consolidated. A cleaner approach in the future would be to use netinfo events to manage Internet reachability as well as specifically checking for the mediator (if configured). Note that even if the mediator is configured, a wallet may still want to exclude or manage the check differently if the mediator is not part core use case.

@jian4on jian4on marked this pull request as ready for review January 15, 2025 20:43
@jian4on jian4on requested a review from timbl-ont January 16, 2025 03:56
@jian4on jian4on marked this pull request as draft January 17, 2025 19:20
@jian4on jian4on marked this pull request as ready for review January 17, 2025 20:58
@jeznorth
Copy link

@jian4on Please reviews comments so we can proceed.

@jian4on jian4on requested a review from al-rosenthal January 21, 2025 22:16
@al-rosenthal al-rosenthal merged commit 52149fd into openwallet-foundation:main Jan 22, 2025
8 checks passed
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.

4 participants