[ABW-1316] Split out signing and submission from TransactionClient, introduce Signing feature (prepares for signing w Ledger)
#455
Conversation
This file contains 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
CyonAlexRDX
changed the title
CyonAlexRDX
commented
Apr 27, 2023
CyonAlexRDX
commented
Apr 27, 2023
| case let .failedToGetTransactionStatus(txID, _): | ||
| return (errorKind: .failedToPollSubmittedTransaction, message: "TXID: \(txID)") | ||
| } | ||
| // case let .failedToSubmit(error): |
There was a problem hiding this comment.
to be fixed... by someone :P
CyonAlexRDX
commented
Apr 27, 2023
| return error.localizedDescription | ||
| case .failedToSubmit: | ||
| return "Failed to submit tx" | ||
| // case let .failedToSubmit(error): |
There was a problem hiding this comment.
to be fixed... by someone :P
GhenadieVP
reviewed
Apr 27, 2023
Sources/Clients/SubmitTransactionClient/SubmitTransactionClient+Interface.swift
Outdated
Show resolved
Hide resolved
CyonAlexRDX
commented
Apr 27, 2023
| if pollCount >= pollStrategy.maxPollTries { | ||
| statusSubject.send(.init( | ||
| txID: txID, | ||
| result: .failure(.failedToGetTransactionStatus( |
There was a problem hiding this comment.
this breaks the while loop, since failure => isComplete: true.
CyonAlexRDX
commented
Apr 27, 2023
| else { | ||
| throw FailedToDeviceFactorSourceForSigning() | ||
| } | ||
| let hdRoot = try loadedMnemonicWithPassphrase.hdRoot() |
There was a problem hiding this comment.
this is a huge improvement, this does not require any "cache" of mnemonics we had earlier, since we sign with all accounts for this hdRoot "in one go".
CyonAlexRDX
commented
Apr 27, 2023
| @@ -0,0 +1,32 @@ | |||
| import FeaturePrelude | |||
|
|
|||
| extension SignWithLedgerFactorSource.State { | |||
There was a problem hiding this comment.
Empty / Not impl for now, to be impl in coming PR
CyonAlexRDX
commented
Apr 27, 2023
| import FeaturePrelude | ||
|
|
||
| // MARK: - SignWithLedgerFactorSource | ||
| public struct SignWithLedgerFactorSource: SignWithFactorReducerProtocol { |
There was a problem hiding this comment.
Empty / Not impl for now, to be impl in coming PR
GhenadieVP
approved these changes
Apr 28, 2023
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
JIRA ABW-1316
This PR does NOT implement signing with Ledger as git branch name might suggest. That will follow in subsequent PR. This PR does however prepare for signing with factor sources which requires UI flow.
This PR splits out:
from TransactionClient
Signing is moved into VIEW layer, a Feature called
Signing.Furthermore, this PR introduces a picker for Fee Payer. It drastically reduces complexity of TransactionClient, by use of an ephemeral
notarykey pair.I've simplified TransactionClient a lot by removing the
Resultstuff. TransactionClient goes from 800 loc to 400 loc. 🎉The best this PR can do is to not break anything and work just as well with the currently only supported factor source:
.device.Demo
Select fee payer
Look at slack
Two signers
This demonstrates signing with TWO accounts.
https://user-images.githubusercontent.com/116169792/235083340-43d88f0d-40e0-4d18-9f9b-60744fe3309f.MOV
CALL TO ACTION
@kugel3 We REALLY REALLY should after this PR get merged change
TransactionReviewReducer, by introducing a new coordinating Reducer called... perhapsTransactwhich coordinates: