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

Flutter bug fixes and enhancements #327

Conversation

quetool
Copy link

@quetool quetool commented Mar 22, 2024

Summary

Why am I opening this PR? I have found an issue on your felix/isConnected-flutter branch (which I assume is the one you used to publish the latest Flutter SDK) where you are using the compilation directive #if DEBUG on your Swift's side plugin and this is breaking the app on Profile/Release mode.

The affected line is this one =>

You have this directive also on branch 2.0 but you don't have in on main branch (the whole guard statement is missing on main branch).

The thing is that by not returning a result() to flutter (because of the compilation directive) on Profile/Release mode the App stucks on await CoinbaseWalletSDK.shared.configure call. This is causing a lot of troubles for users.

So, this PR basically merges the changes from felix/isConnected-flutter to main (without the compilation directive) and adds a few more valuable things.

Such as:

  • Updated dependencies
  • Added ownPublicKey and peerPublicKey methods (Android side should be checked properly)
  • Added WatchAsset() object for rpc request
  • Updated example files accordingly

Furthermore, it raise a concern in regards of the whole CoinbaseWalletSDK.shared.configure method, which is: Do we really needed? My conclusion is no.
configure method shouldn't be called from Flutter side since the calling could happen too late raising an error when opening the app from terminated state.

So, since Flutter requires anyway iOS platform specific code https://github.com/MobileWalletProtocol/wallet-mobile-sdk/tree/main/flutter#ios-only it makes sense to call configure on native side as well as it is currently done for iOS native SDK https://github.com/MobileWalletProtocol/wallet-mobile-sdk/tree/main/ios#setup
Same situation for Android https://github.com/MobileWalletProtocol/wallet-mobile-sdk/tree/main/android#setup

How did you test your changes?

  • Smoke tests and manual tests

bangtoven and others added 23 commits March 10, 2023 13:51
… type, version check (MobileWalletProtocol#303)

* update pod version

* introduce VerificationMethod

* introducing verification method

* error

* client sample

* version bump

* backward compatibility

* more robust way to check UnsupportedAction

* Revert "backward compatibility"

This reverts commit 3871e11.

Revert "client sample"

This reverts commit e973520.

Revert "introducing verification method"

This reverts commit a5c40ce.

Revert "introduce VerificationMethod"

This reverts commit 2cc4c3f.

* getCoinbaseWalletMWPVersion

* update hasUnsupportedAction

* introduce optional actionSource arg on eth_sendTransaction

* cleanup

* cleanup

* CoinbaseWalletSDK.getCoinbaseWalletMWPVersion

* version up

* bump deploy target to ios 13

* update github org name
…l#311)

* Add ActionSource to eth_sendTransaction

* Copy over Kotlin serialization changes from 2.0 branch
* update build.gradle

* embed public data
Bumps [postcss](https://github.com/postcss/postcss) from 8.4.18 to 8.4.35.
- [Release notes](https://github.com/postcss/postcss/releases)
- [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md)
- [Commits](postcss/postcss@8.4.18...8.4.35)

---
updated-dependencies:
- dependency-name: postcss
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [semver](https://github.com/npm/node-semver) from 5.7.1 to 5.7.2.
- [Release notes](https://github.com/npm/node-semver/releases)
- [Changelog](https://github.com/npm/node-semver/blob/v5.7.2/CHANGELOG.md)
- [Commits](npm/node-semver@v5.7.1...v5.7.2)

---
updated-dependencies:
- dependency-name: semver
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…tocol#307)

Bumps [ip](https://github.com/indutny/node-ip) from 1.1.8 to 1.1.9.
- [Commits](indutny/node-ip@v1.1.8...v1.1.9)

---
updated-dependencies:
- dependency-name: ip
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ocol#309)

Bumps [@babel/traverse](https://github.com/babel/babel/tree/HEAD/packages/babel-traverse) from 7.20.1 to 7.23.9.
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.23.9/packages/babel-traverse)

---
updated-dependencies:
- dependency-name: "@babel/traverse"
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…tocol#308)

Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.2 to 1.15.5.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.15.2...v1.15.5)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…rotocol#313)

Bumps [http-cache-semantics](https://github.com/kornelski/http-cache-semantics) from 4.1.0 to 4.1.1.
- [Commits](kornelski/http-cache-semantics@v4.1.0...v4.1.1)

---
updated-dependencies:
- dependency-name: http-cache-semantics
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [webpack](https://github.com/webpack/webpack) from 5.74.0 to 5.90.3.
- [Release notes](https://github.com/webpack/webpack/releases)
- [Commits](webpack/webpack@v5.74.0...v5.90.3)

---
updated-dependencies:
- dependency-name: webpack
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…col#314)

Bumps [@sideway/formula](https://github.com/sideway/formula) from 3.0.0 to 3.0.1.
- [Commits](hapijs/formula@v3.0.0...v3.0.1)

---
updated-dependencies:
- dependency-name: "@sideway/formula"
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [json5](https://github.com/json5/json5) from 2.2.1 to 2.2.3.
- [Release notes](https://github.com/json5/json5/releases)
- [Changelog](https://github.com/json5/json5/blob/main/CHANGELOG.md)
- [Commits](json5/json5@v2.2.1...v2.2.3)

---
updated-dependencies:
- dependency-name: json5
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…l#316)

Bumps [ua-parser-js](https://github.com/faisalman/ua-parser-js) from 0.7.32 to 0.7.37.
- [Release notes](https://github.com/faisalman/ua-parser-js/releases)
- [Changelog](https://github.com/faisalman/ua-parser-js/blob/master/CHANGELOG.md)
- [Commits](faisalman/ua-parser-js@0.7.32...0.7.37)

---
updated-dependencies:
- dependency-name: ua-parser-js
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Re-export types in react-native/client (MobileWalletProtocol#297)

* Update CoinbaseWalletSDK.types.ts (MobileWalletProtocol#298)

Match runtime types

---------

Co-authored-by: Tony D'Addeo <[email protected]>
* RN SDK update

* add action source

* add getCoinbaseWalletMWPVersion

* ensure main thread

* expose swift error correctly

* getCoinbaseWalletMWPVersion on Android

* remove tsconfig
…ranch, added ownPublicKey and peerPublicKey methods, added AddEthereumChain and WatchAsset rpc methods, updated example accordingly
@fan-zhang-sv fan-zhang-sv changed the base branch from main to 2.0 April 15, 2024 22:45
@fan-zhang-sv fan-zhang-sv requested a review from a team as a code owner April 15, 2024 22:45
@fan-zhang-sv fan-zhang-sv changed the base branch from 2.0 to main April 15, 2024 22:45
@fan-zhang-sv fan-zhang-sv changed the base branch from main to 2.0 April 16, 2024 05:09
@fan-zhang-sv fan-zhang-sv changed the base branch from 2.0 to main April 16, 2024 05:10
Copy link

@fan-zhang-sv fan-zhang-sv left a comment

Choose a reason for hiding this comment

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

Ty for putting up this fix!

Code looks good, agree on we can configure at native level, could you switch the PR to be against 2.0 branch, we will release Flutter from 2.0 branch to unblock your team.


private fun peerPublicKey(@NonNull result: Result) {
// TODO implement the proper function to return coinbase.peerPublicKey
result.success("")

Choose a reason for hiding this comment

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

same for result.success(coinbase.peerPublicKey)?

Copy link
Author

Choose a reason for hiding this comment

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

It should indeed, as it is the same of coinbase.isCoinbaseWalletInstalled and coinbase.isConnected. They are properties of CoinbaseWalletSDK of the native android SDK but when I tried to access them that way I received a compile error and since I'm not expert on Android I figured I just leave it up to you 😅
My guess is that it would have to be deployed first the coinbase-wallet-sdk for native Android with this changes to maven and then add the dependency in the Flutter SDK's Android side.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @quetool, I've updated the Android SDK to expose the ownPublicKey and peerPublicKey fields. Would you mind updating your PR to pull in the latest version of the SDK?

implementation "com.coinbase:coinbase-wallet-sdk:1.1.1"

Copy link
Author

Choose a reason for hiding this comment

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

Hello @vishnumad! It seems like you made the changes in main branch but @fan-zhang-sv asked me to switch to 2.0 in our latest interaction. Is it ok to full from main into this PR that points to 2.0 branch?

Copy link
Contributor

Choose a reason for hiding this comment

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

@quetool I made the release off the main branch, but the change should be in the 2.0 branch as well. In your PR, you’ll need to update the Flutter SDK build.gradle file to pull the latest version:

implementation "com.coinbase:coinbase-wallet-sdk:1.0.4"

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! It's done!


private fun ownPublicKey(@NonNull result: Result) {
// TODO implement the proper function to return coinbase.ownPublicKey
result.success("")

Choose a reason for hiding this comment

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

result.success(coinbase.ownPublicKey) should work i think?

Copy link
Author

Choose a reason for hiding this comment

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

@quetool quetool changed the base branch from main to 2.0 April 16, 2024 07:47
@quetool quetool changed the base branch from 2.0 to main April 16, 2024 07:47
@quetool quetool requested a review from bangtoven as a code owner April 16, 2024 08:18
@quetool quetool changed the base branch from main to 2.0 April 16, 2024 08:18
@bangtoven bangtoven requested review from vishnumad and removed request for jakubuid June 25, 2024 17:53
Copy link
Contributor

@vishnumad vishnumad 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 to me, thank you @quetool!

@vishnumad vishnumad merged commit d7e0bfc into MobileWalletProtocol:2.0 Jun 27, 2024
@quetool
Copy link
Author

quetool commented Jun 27, 2024

Looks good to me, thank you @quetool!

Thank you very much @vishnumad! Let me know when the new version is published over pub.dev 💪

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.

6 participants