simonmcl
commented
2 years ago @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.subVerifierIdtoverifierName. The constructor takes in a parameter namedverifierNameas 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
Hi @rathishubham7 thanks for releasing version 1.0.1, however most of the issues still remain
Solved:
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 641has 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
FetchNodeDetails
AllNodeDetailshas many force unwrapsNodeDetailswas split intoAllNodeDetailsandNodeDetails, there are no code comments on the classes or the functions to explain the difference between thesefinalDocumentation:
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
privateandfinalkeywords throughout the library, and instead useopen. 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-sdkRe-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
NetworkServicewhich takes in aURLSession: https://github.com/kukai-wallet/kukai-core-swift/blob/main/Sources/KukaiCoreSwift/Services/NetworkService.swift#L43 , Then in aMockConstantsfile 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 ofURLProtocolthat 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
verifierNameis needed in part of the code but is inaccessible, I have to store that data along side the theSubVerifierDetailsobject 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.