You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Right now, the output from the TransactionBuilder is an UnsignedTx, which contains all the material needed to produce a Tx that can be submitted to consensus, with the exception of anything that requires the private account keys. This allows separating the majority of transaction construction and transaction signing, and provides the groundwork for hardware wallet support. This separation makes the full-service hardware wallet support possible.
There's an important caveat here: the output TxOuts, together with their memos, are created and bundled in the UnsignedTx. For most memos that is fine, however this means that for authenticated sender memos, which require the spend private key, the caller has two options:
Have access to the spend private view key when producing the UnsignedTx, so that they can sign their authenticated sender memo (for example, mobilecoind does this)
Not create an RTH authenticated sender memo
This is not ideal since we want all of our clients, including full-service with it's hardware wallet support, to construct RTH authenticated sender memos.
The solution I am proposing in this PR is a necessary evil to enable this. In this PR, we move away from having the TransactionBuilder output an UnsignedTx, to having it output a new object I have named TxBlueprint.
The TxBlueprint and UnsignedTx contain mostly similar information, with the main difference being that a TxBlueprint contains information on how to build the output TxOuts, but does not contain the actual output TxOut themselves. The important implication of this is that the TxBlueprint can be constructed without a MemoBuilder, which means it does not need access to any sensitive key material (remember: prior to this PR, the RTHMemoBuilder requires a SenderMemoCredential instance, which requires the spend private key!).
A TxBlueprint, together with a MemoBuilder instance, can be converted into an UnsignedTx, and from there the flow is similar to how things work today (the UnsignedTx needs to be signed in order to produce a Tx that can be sent to consensus).
To reiterate, the flow today is:
Create a TransactionBuilder, passing a MemoBuilder. If RTH Authenticated Sender memos are desirable, access to the private spend key is needed at this stage.
Use the TransactionBuilder to create an UnsignedTx
(Optionally move UnsignedTx to a different machine/service)
The UnsignedTx can be signed with an AccountKey or a hardware wallet, producing a Tx
After this. change, the flow will be:
Create a TransactionBuilder
Use TransactionBuilder to produce a TxBlueprint
(Optionally move TxBlueprint to a different machine/service)
Convert TxBlueprint into an UnsignedTx by providing a MemoBuilder (which may require access to the sensitive key material)
The UnsignedTx can be signed with an AccountKey or a hardware wallet, producing a Tx.
Helper methods make this change a little less intrusive than it might sound. However, there are some important side-effect worth mentioning:
Right now, when outputs are added to a TransactionBuilder, memos are constructed and the MemoBuilder could return an error. For example, adding duplicate change outputs is not supported by all memo builders, and attempting to add more than one change output will result in an error when calling add_change_output. SInce memo building is now being postponed to a later stage, this will not be caught early. The user will still encounter the error, but only when they attempt to go from a TxBlueprint to an UnsignedTx. In my opinion this is an acceptable tradeoff since almost all cases (and essentially in all cases where we use TransactionBuilder ourselves), we do not expect to encounter these errors - these errors indicate misuse of the TransactionBuilder API.
Right now, when outputs are added to a TransactionBuilder, the caller gets the final TxOut that will be included in the transaction. Since we're not producing that TxOut at this stage, this is no longer possible. Instead, the caller gets the TxOut public key, which makes it easy for them to later find the TxOut if they need to (after obtaining the UnsignedTx from the TxBlueprint). This hasn't proved to be problematic in this repo and in full-service, and I suspect will not be problematic in the mobile SDKs either.
I want to mention one other potential approach that was considered: One other approach here is to not modify TransactionBuilder as excessively as I've done here, and instead figure out some hack to replace the Authenticated Sender Memo of the output TxOut before signing a transaction. This will be a much smaller change, but I think it's an unreasonable approach - having some caveat that in certain cases a TxOut might change in an UnsignedTx just before signing seems like a major foot gun. As such, I went with the much more elaborate TxBlueprint approach.
I'm open to other suggestions, but the reality is that I don't think there's an elegant solution here because the memos right now get created very early on, and it's possible for a memo to require access to private key material, so that has to move "up the stack" to where signing takes place.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Right now, the output from the
TransactionBuilderis anUnsignedTx, which contains all the material needed to produce aTxthat can be submitted to consensus, with the exception of anything that requires the private account keys. This allows separating the majority of transaction construction and transaction signing, and provides the groundwork for hardware wallet support. This separation makes thefull-servicehardware wallet support possible.There's an important caveat here: the output
TxOuts, together with their memos, are created and bundled in theUnsignedTx. For most memos that is fine, however this means that for authenticated sender memos, which require the spend private key, the caller has two options:UnsignedTx, so that they can sign their authenticated sender memo (for example,mobilecoinddoes this)This is not ideal since we want all of our clients, including
full-servicewith it's hardware wallet support, to construct RTH authenticated sender memos.The solution I am proposing in this PR is a necessary evil to enable this. In this PR, we move away from having the
TransactionBuilderoutput anUnsignedTx, to having it output a new object I have namedTxBlueprint.The
TxBlueprintandUnsignedTxcontain mostly similar information, with the main difference being that aTxBlueprintcontains information on how to build the outputTxOuts, but does not contain the actual outputTxOutthemselves. The important implication of this is that theTxBlueprintcan be constructed without aMemoBuilder, which means it does not need access to any sensitive key material (remember: prior to this PR, theRTHMemoBuilderrequires aSenderMemoCredentialinstance, which requires the spend private key!).A
TxBlueprint, together with aMemoBuilderinstance, can be converted into anUnsignedTx, and from there the flow is similar to how things work today (theUnsignedTxneeds to be signed in order to produce aTxthat can be sent to consensus).To reiterate, the flow today is:
TransactionBuilder, passing aMemoBuilder. If RTH Authenticated Sender memos are desirable, access to the private spend key is needed at this stage.TransactionBuilderto create anUnsignedTxUnsignedTxto a different machine/service)UnsignedTxcan be signed with anAccountKeyor a hardware wallet, producing aTxAfter this. change, the flow will be:
TransactionBuilderTransactionBuilderto produce aTxBlueprintTxBlueprintto a different machine/service)TxBlueprintinto anUnsignedTxby providing aMemoBuilder(which may require access to the sensitive key material)UnsignedTxcan be signed with anAccountKeyor a hardware wallet, producing aTx.Helper methods make this change a little less intrusive than it might sound. However, there are some important side-effect worth mentioning:
TransactionBuilder, memos are constructed and theMemoBuildercould return an error. For example, adding duplicate change outputs is not supported by all memo builders, and attempting to add more than one change output will result in an error when callingadd_change_output. SInce memo building is now being postponed to a later stage, this will not be caught early. The user will still encounter the error, but only when they attempt to go from aTxBlueprintto anUnsignedTx. In my opinion this is an acceptable tradeoff since almost all cases (and essentially in all cases where we useTransactionBuilderourselves), we do not expect to encounter these errors - these errors indicate misuse of theTransactionBuilderAPI.TransactionBuilder, the caller gets the finalTxOutthat will be included in the transaction. Since we're not producing thatTxOutat this stage, this is no longer possible. Instead, the caller gets theTxOutpublic key, which makes it easy for them to later find theTxOutif they need to (after obtaining theUnsignedTxfrom theTxBlueprint). This hasn't proved to be problematic in this repo and infull-service, and I suspect will not be problematic in the mobile SDKs either.I want to mention one other potential approach that was considered: One other approach here is to not modify
TransactionBuilderas excessively as I've done here, and instead figure out some hack to replace the Authenticated Sender Memo of the outputTxOutbefore signing a transaction. This will be a much smaller change, but I think it's an unreasonable approach - having some caveat that in certain cases aTxOutmight change in anUnsignedTxjust before signing seems like a major foot gun. As such, I went with the much more elaborateTxBlueprintapproach.I'm open to other suggestions, but the reality is that I don't think there's an elegant solution here because the memos right now get created very early on, and it's possible for a memo to require access to private key material, so that has to move "up the stack" to where signing takes place.