Electric-Coin-Company / zcash-android-wallet-sdk

Native Android SDK for Zcash
MIT License
5 stars 10 forks source link

Expose the transaction proposal in a way that enables querying its fee #1359

Closed str4d closed 8 months ago

str4d commented 9 months ago

See https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/pull/1344#issuecomment-1896694025

str4d commented 8 months ago

In https://github.com/zcash/librustzcash/pull/1187 @nuttycom is generalising the proposal structure to potentially create multiple transactions. Integrating this into the SDK is mostly straightforward up until we reach SdkSynchronizer. This is what we currently do in SdkSynchronizer.sendToAddress: https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/blob/a04f6566f85553132f9866bcff162a223e290381/sdk-lib/src/main/java/cash/z/ecc/android/sdk/SdkSynchronizer.kt#L561-L574

(This returns an internal database index, but I think we should be returning a txid instead.)

Here we are creating a transaction (which places it into the wallet DB, and thus into the wallet's transaction history), and then if we fail to broadcast the transaction we return an error. After the proposals change, we will have multiple transactions to submit here, and they may individually succeed or fail.

I think it would be a bad API to throw an error if the second or later transaction fails to be submitted, because unlike before this would mean the proposal was partially implemented, and the caller should be informed of this. However, we also can't at this layer assume that we can submit later transactions if the earlier ones fail, because they are expected to form a chain rather than be a set of independent transactions.

I think we maybe need some Result-like API (whatever the Kotlin version of this is) that returns a list of transaction IDs in the success case, and in the failure case:

HonzaR commented 8 months ago

Yes, returning a sealed class would make sense here instead of id: Long. The returned Long value is not used in the Zashi wallet for anything other than logging now.

Example of such an API:

sealed class SendTransactionResult {

    data class Success(val txIds: List<Long>) : SendTransactionResult()

   // Entire transaction submission is not available
   data class Failure(val reason: TransactionSubmitException): SendTransactionResult()

    data class Partial(
       val submittedTxIds: List<Long>, 
       val createdTxIds: List<Long>, 
       val failedTxs: List<FailedTx>
    ) : SendTransactionResult()

and

data class FailedTx(
   val txId: Long,
   val reason: TransactionSubmitException
)
str4d commented 8 months ago

In a pairing today, @nuttycom and I decided that we will change Synchronizer.proposeShielding in three ways:

The transparent receiver argument serves two functions: