Version 1.0.1 issues

[simonmcl]

Hi @rathishubham7 thanks for releasing version 1.0.1, however most of the issues still remain

Solved:

• Warning from starscream has been removed
• FetchNodeDetails function is no longer blocking main thread

Unsolved / new issues:

• The SDK still crashes when running on my real device. While you removed the specific force unwraps I pointed out, there are many still present or new ones been added. TorusUtils+extension line 641 has a force unwrap try statement (try!), to try parse the network response. If the network response differs in any way, the entire app crashes. This is again crashing my app as the network response is not what you expect it to be, this is incredibly dangerous for a number of reasons.

• TDSDKFactory

  • This doesn't provide a solution to my need to mock the network requests. The classes it wraps around are final classes which can't be subclassed, and have private properties and functions which need to be changed.
  • The TrousDirectSDK constructor that allows you to specify the logging level, requires to pass in an instance of an object conforming to the protocol. The one supplied by the package (TDSDKFactory) has an inaccessible constructor, so I can't use the in-built one for my non-mocked versions of the code
  • This parameter on the TrousDirectSDK constructor should probably be defaulted to the inbuilt class provided

• FetchNodeDetails

  • AllNodeDetails has many force unwraps
  • I'm not understanding why NodeDetails was split into AllNodeDetails and NodeDetails, there are no code comments on the classes or the functions to explain the difference between these
  • I can't subclass FetchNodeDetails because its been marked as final

• Documentation:

  • The documentation hasn't been updated to reflect the new changes, showing devs how to use the new factory for example. Some of the above issues may have been caught if it was added to the demo project
  • There are also no examples on how to use FetchNodeDetails
  • Theres no code comments on any of the functions or objects, some of the naming is not clear and hard to follow without comments

Solutions / discussions

• Mocking / dependency injection:
  If you are going down the road of providing developers with the ability to provide their own instances of classes in order to mock (or to implement custom logic), then you will need to remove the majority of the private and final keywords throughout the library, and instead use open. I had to do this in 3 forks of the libraries I made as a temporary workaround to the issues, you can have a look at the most recent commits here: https://github.com/simonmcl/fetch-node-details-swift, https://github.com/simonmcl/torus-utils-swift, https://github.com/simonmcl/torus-direct-swift-sdk

  Re-iterating my previous point from the other ticket, for mocking it would be much simpler to expose a URLSession instance and allow developers to just stub the network requests. Forcing developers to have to learn all these classes + figure out the models, is a huge amount fo effort compare to just turning on logging and copying the network requests into stub files.

  E.g. In my package I have all my networking run through NetworkService which takes in a URLSession: https://github.com/kukai-wallet/kukai-core-swift/blob/main/Sources/KukaiCoreSwift/Services/NetworkService.swift#L43 , Then in a MockConstants file I simply make a dictionary of URLs and stub json files: https://github.com/kukai-wallet/kukai-core-swift/blob/main/Tests/KukaiCoreSwiftTests/MockConstants.swift#L67 . I have a mock version of URLProtocol that simply matches the URLs and returns the stub file content. Thats all thats needed to mock the entire library and all its dependencies in a handful of lines of code.

• PromiseKit
  PromiseKit and PromiseKit/Foundation are the largest downloads by a long shot. Importing TorusDirectSDK hangs on downloading PromiseKit for quite some time compared to the rest. I'm not using that in my library and would prefer to not have to force it onto everyone making use of mine, given its not necessary. It would be great if this could be made optional, by just using DispathQueue's and completion blocks in the main repo. If other users want PromiseKit, a second repo TorusDirectSDK+PromiseKit could be made, with extensions on the objects to return promises. This would avoid a very large dependency for those that don't need it. This is the same pattern that the PromiseKit developers follow. Here is a standalone repo to add PromiseKit definitions to Alamofire: https://github.com/PromiseKit/PMKAlamofire

• Making properties public
  Separate to the mocking discussion, a lot of properties that I would like to access are not marked public and are inaccessible. The Subverifier's property verifierName is needed in part of the code but is inaccessible, I have to store that data along side the the SubVerifierDetails object in a complex dictionary, so I can retrieve it later. Objects that developers have to touch, in most cases, should have all their properties public so they don't have to store 2 copies of the same data.

[simonmcl]

@rathishubham7 @michaellee8

Ok so the crashes have indeed been fixed, the custom logging removed and the dependencies updated so theres no longer any Xcode warnings / issues. Thanks.

However I still ran into a number of issues

• Instead of crashing, the SDK fails to return any errors and will just hang forever. There are console warnings from PromiseKit saying a promise was deallocated before it was called, whenever an error is encountered

  PromiseKit: warning: pending promise deallocated
  PromiseKit: warning: pending promise deallocated
  PromiseKit: warning: pending guarantee deallocated
  PromiseKit: warning: pending guarantee deallocated
  PromiseKit: warning: pending guarantee deallocated
  

• Theres also still no public init for TDSDKFactory, meaning I have to duplicate the one inside the SDK in order to make use of the TorusSwiftDirectSDK constructor

• The factory approach still doesn't address my mocking needs, as classes, constructors and methods are not marked as open

  

• Can you rename SubverifierDetails.subVerifierId to verifierName. The constructor takes in a parameter named verifierName as do many of the other methods. But the underlying property is named something else. I didn't realise you had fixed the issue regarding private properties here, as I couldn't find the property

• I encountered a few UI/UX issues with the webview, i'll upload videos/ pictures to the telegram chat

Also please let me know if there is a plan to look at some of the bigger items mentioned in the first post above, like making PromiseKit optional and being able to pass in a URLSession instead of relying on the Factory approach. As mentioned previously, these are not ideal and continuing to cause problems every release

[s1dc0des]

TorusUtils+extension line 641

solve this please..