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

Use Singleton pattern on BreezSDK using factory constructor #863

Closed
wants to merge 2 commits into from

Conversation

erdemyerebasmaz
Copy link
Contributor

@erdemyerebasmaz erdemyerebasmaz commented Mar 15, 2024

Apply Singleton pattern on BreezSDK using factory constructor.

  • Move SdkConfig extension to it's own file.
  • Export bridge_generated.dart, breez_sdk.dart, exceptions.dart & sdk_config.dart files through sdk.dart
    • Filter out generated binding classes from bridge_generated.dart

I assumed an SDK partner encountered this issue because we were lazily creating a new instance on our documentation for Dart snippets and if taken as 1:1 example, it's likely to encounter this issue. However, it may very well be a symbol stripping issue on iOS.

Calling this function returns an error:
await BreezSDK().mnemonicToSeed(mnemonicCode);

Exception: Subclasses of
'FlutterRustBBreezSdkCorePlatform singletons - there should not be two instances
(runtimeType=BreezSdkCorePlatform)

Either way, I expect these changes to

  • take away the resource/instance management responsibility away from users,
  • reduce import statements,
  • simplify API access.

- Move SDK config extension to it's own file.
- Export bridge_generated.dart, breez_sdk.dart, exceptions.dart & sdk_config.dart files through sdk.dart
- Filter out generated binding classes from bridge_generated.dart
@erdemyerebasmaz
Copy link
Contributor Author

erdemyerebasmaz commented Mar 15, 2024

App has no issues but tests are failing with

Invalid argument(s): Failed to lookup symbol 'store_dart_post_cobject': dlsym(RTLD_DEFAULT, store_dart_post_cobject): symbol not found

on C-Breez, which needs to be addressed before this PR gets merged. stacktrace

Related: Testing Library functions in a flutter package with Native Functions using Dart::ffi

@erdemyerebasmaz
Copy link
Contributor Author

erdemyerebasmaz commented Mar 15, 2024

App has no issues but tests are failing with

Invalid argument(s): Failed to lookup symbol 'store_dart_post_cobject': dlsym(RTLD_DEFAULT, store_dart_post_cobject): symbol not found

on C-Breez, which needs to be addressed before this PR gets merged. stacktrace

Related: Testing Library functions in a flutter package with Native Functions using Dart::ffi

Actually, after investigating our test cases & how they are structured on C-Breez, I can say this is not a blocker. This problem exists on our current test cases as well, but since we were passing BreezSdkMock to Bloc's and no function is called through BreezSdk, it's hidden very well.

To overcome this issue you need to build libraries for the platform you're on and move them to a directory accessible from test folder where you run flutter test.

This local library path can be configured on the Flutter plugin & used through a statement similar to the following we'll be adding to _getDynamicLibrary.

if (Platform.environment.containsKey('FLUTTER_TEST')) {
    return DynamicLibrary.open('$CUSTOM_LIB_PATH');
} else if (Platform.isAndroid ....

Edit: I've been running C-Breez tests with "../breez-sdk/libs/target/aarch64-apple-darwin/release/libbreez_sdk_bindings.dylib" set as library path on my Mac and this in my opinion is an improvement over doing tests with mock data.

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.

@erdemyerebasmaz I agree with fixing the imports and testing with the real library instead of mock data. I am a bit hesitating regarding the singleton at this level.
Given the original user error was due to symbol stripping, can you elaborate on what moving to singleton fixes here?

@erdemyerebasmaz
Copy link
Contributor Author

erdemyerebasmaz commented Mar 16, 2024

@erdemyerebasmaz I agree with fixing the imports and testing with the real library instead of mock data. I am a bit hesitating regarding the singleton at this level. Given the original user error was due to symbol stripping, can you elaborate on what moving to singleton fixes here?

That's true but I believe the issue can be reproduced if there's more than once instance of BreezSDK running on app layer. I will be running tests to make it clear that exactly one instance of BreezSDK(well, BreezSDKCore to be precise) is required.

By moving to Singleton,

This is common practice on a large percent of plugins with similar requirements. We can improve the current implementation by reviewing how they addressed the shortcomings of using Singleton pattern, or move it down a layer to where we initialize BreezSDKCore( on native_toolkit.dart).


Tests can still be mocked using dependency injection.

class Breez {
  // declare a BreezSDK property and pass it as a constructor argument
  const Breez (this._sdk);
  final BreezSDK _sdk;

  // use it when needed
  Future<void> connect() => _sdk.connect();
}

// declare a mock class that implements the type of our dependency
class BreezSDKMock extends Mock implements BreezSDK{}

test('calls connect', () async {
  // create the mock dependency
  final mock = BreezSDKMock();
  // stub its method(s) to return a value when called
  when(mock.connect).thenAnswer((_) => Future.value());
  // create the object under test and pass the mock as an argument
  final breez = Breez(mock);
  // call the desired method
  await breez.connect();
  // check that the method was called on the mock
  expect(mock.connect).called(1);
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants