Consider Repackaging SDK Classes

[ccjernigan]

What is your feature request? Currently the SDK public APIs, as well as SDK internal implementation details, are not clearly disambiguated in the package structure of the SDK. This is a feature request to make that more clear.

It could be implemented by moving the public APIs to their own package (e.g. .api) or moving the internal APIs to their own package (e.g. .internal).

Using the .internal approach is more likely to be able to avoid breaking changes to SDK consumers.

How would this feature help you? As an SDK maintainer, it will be much more clear what changes would be a breaking API for SDK clients. We could also choose to apply ProGuard to the non-public package when publishing, further obfuscating it so that it isn't readily visible to touch.

As an SDK consumer, it will be much more clear what is the actual surface area of the SDK.

Below is an initial rough draft plan on how to go about this. This plan could be broken up into multiple sub issues and done in multiple pull requests. For now, this is intended to be an outline for the overall approach.

Phase 1 #293 Refactor the truly private APIs that aren't being used by the wallet

• cash.z.ecc.android.sdk.db.CompactBlockDb
• cash.z.ecc.android.sdk.db.DerivedDataDb
• cash.z.ecc.android.sdk.db.PendingTransactionDb
• cash.z.ecc.android.sdk.db.CompactBlockDb
• cash.z.ecc.android.sdk.ext
  • android.*
  • Ext
  • BlockExt - Refactor toHexReversed method (don't refactor toHex() which is used by the wallet)
  • WalletService
  • Placeholders
  • Sma
• cash.z.ecc.android.sdk.service.*
• cash.z.ecc.android.sdk.tool.SaplingParamTool
• cash.z.ecc.android.sdk.transaction.*

Phase 2 #294 Copy and paste utilities from the SDK into the wallet app's codebase. They're useful, but not part of the SDK's public API.

• cash.z.ecc.android.sdk.ext.Twig

Phase 3 #295 Refactoring with minor public API changes

• cash.z.ecc.android.sdk.block.* - This entire package appears to be internal implementation details, with the exception of cash.z.ecc.android.sdk.block.CompactBlockProcessor.ProcessorInfo. Recommend refactoring the ProcessorInfo class out.
• cash.z.ecc.android.sdk.jni.* - RustBackend is used by a single Wallet unit test, which should be refactored. Also the interface RustBackendWelding.Derivation is used by DerivationTool. Should refactor RustBackendWelding.Derivation elsewhere.
• cash.z.ecc.android.sdk.type.ConsensusMatchType - Does not appear to be used by the wallet app, although is exposed by Synchronizer.validateConsensusBranch(). Also cash.z.ecc.android.sdk.ext.ConsensusBranchId, which is used by ConsensusMatchType

Phase 4 #296 Refactor with major API changes

• cash.z.ecc.android.sdk.exception - While the Wallet app is currently using some of these exceptions, some of them are implementation details of the SDK and should probably be refactored in a way that doesn't expose them to SDK clients. Specifically this means:
  • LightWalletException
  • CompactBlockProcessorException
• cash.z.ecc.android.sdk.db.entity.* - While these classes are used by the wallet app, they're actually our SDK's internal representation of the database model. We should consider creating a separate copy of these objects to vend from the SDK, hiding the database implementation details from clients.
• cash.z.ecc.android.sdk.SdkSynchronizer - The Wallet is casting the Synchronizer to SdkSynchronizer in a few places, which needs to be fixed. In addition to fixing that, we should move the initializer from SdkSynchronizer to Synchronizer.

[pacu]

I'd like to pair up with you to know which are the public APIs you are referring and also evaluating making those changes in the iOS SDK too if applicable

[ccjernigan]

Looking forward to collaborating.

To make the idea more clear, this is an example using the entry point to the SDK: Synchronizer is an interface and SdkSynchronizer is the default implementation of that interface. Both are public and in the same package. A top level function is used as the factory to create SdkSynchronizer and this function lives in the SdkSynchronizer.kt file.

One possible refactoring of this:

• Keep Synchronizer where it is
• Move SdkSynchronizer to a .internal package to make it more clearly non-public
• Add a Companion object to Synchronizer with a factory function, like new or default

Clients would then call Synchronizer.new().

The public API now lives in Synchronizer, and both maintainers and consumers have a much clearer idea as to the public API contract.

This would be a breaking API change. There are some things we can do, such as keeping the existing top level initializer and marking it as deprecated during a migration period.

This is just one example, and we'll need to work through the public API surface area on both platforms to identify what other APIs we should consider doing this for.

[pacu]

I think these two components. will actually be used in the next app. so they might stay where they are

cash.z.ecc.android.sdk.type.UnifiedAddressAccount
cash.z.ecc.android.sdk.type.UnifiedAddress

I see the rest being fine. Nobody actually used the compact block processor on it's own so I think it's fair to actually make it an internal component