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

Remove Breez SDK dependency #29

Merged
merged 6 commits into from
Jun 27, 2024
Merged

Remove Breez SDK dependency #29

merged 6 commits into from
Jun 27, 2024

Conversation

erdemyerebasmaz
Copy link
Collaborator

@erdemyerebasmaz erdemyerebasmaz commented Jun 26, 2024

Run CI
Depends on:


This PR removes Breez SDK as shared modules are fully moved to sdk-common and is now accessible from Liquid SDK.

Changelist:

  • Update CurrencyBloc to use FiatAPI
    • Re-enable CurrencyConverterDialog
  • Integrate input parser
    • We recognize almost all input types at the moment but input handling will be enabled as part of another PR: Handle LNURL's #30
  • Remove Breez SDK setup steps from CI workflow
  • Update dependencies to latest

@@ -28,7 +25,7 @@ class CurrencyBloc extends Cubit<CurrencyState> with HydratedMixin {
}

void listFiatCurrencies() {
_breezSDK.listFiatCurrencies().then((fiatCurrencies) {
liquidSdk.wallet!.listFiatCurrencies().then((fiatCurrencies) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this isn't under review (sorry for throwing the 🔧). I think name wallet might become confusing for devs using the SDK, especially when listing fiat rates etc. They might assume its access to the underlying liquid wallet. Something like instance might be better (if it's not a reserved word) liquidSdk.instance!.listFiatCurrencies()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. Let's open an issue for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

inputType is InputType_LnUrlWithdraw ||
inputType is InputType_LnUrlAuth ||
inputType is InputType_LnUrlError ||
inputType is InputType_NodeId)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think NodeId would also not be supported

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noted. It'll be removed as part of #30

Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

LGTM

@erdemyerebasmaz
Copy link
Collaborator Author

Thanks for the reviews. I'll merge after adding CI requirements.

*Shared modules are moved to sdk-common and is now accessible from Liquid SDK.

*Update CurrencyBloc to use FiatAPI
- Remove setting up Breez SDK steps
- Remove choosing a Breez SDK ref for workflow
@erdemyerebasmaz erdemyerebasmaz merged commit 6aa030c into main Jun 27, 2024
2 checks passed
@erdemyerebasmaz erdemyerebasmaz deleted the breez-sdk-removal branch June 27, 2024 09:25
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.

Integrate FiatCurrency Remove Breez SDK dependency Integrate Input Parser
3 participants