-
Notifications
You must be signed in to change notification settings - Fork 14
chore: Integrate joint account support into core transaction signing #514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: multisig/03-tests-and-deeplink
Are you sure you want to change the base?
chore: Integrate joint account support into core transaction signing #514
Conversation
b98a876 to
5211943
Compare
8602ae8 to
cfd09c6
Compare
- Fix duplicate DI bindings for joint account repository - Add missing interface implementations - Update transaction signer handling for joint accounts - Fix import statements and code organization
cfd09c6 to
252f8ea
Compare
- Replace GetJointAccountParticipantCount with GetLocalAccount - Update JointSignRequestDTO to JointSignRequest - Get participant count from LocalAccount.Joint.participantAddresses
Convert TODO() to comment-style TODOs and remove duplicate when cases to fix detekt UnreachableCode violations. Files fixed: - GetAccountOriginalStateIconDrawablePreviewUseCase - AccountStatusDetailPreviewDecider - CreateWalletConnectArbitraryDataSignerUseCase - GetAccountIconDrawablePreviewUseCase
| suspend fun getLocalAccount(address: String): LocalAccount? { | ||
| return getLocalAccount.invoke(address) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary function. We can remove this and inject use case to class which uses this function
|
|
||
| class JointAccountTransactionSignHelper @Inject constructor( | ||
| private val getLocalAccount: GetLocalAccount, | ||
| private val getLocalAccounts: GetLocalAccounts, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused parameter
| } | ||
| } | ||
|
|
||
| private suspend fun signWithAlgo25Account(transactionBytes: ByteArray, signerAddress: String): ByteArray? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LocalAccountSignHelper can be used instead
| } | ||
| } | ||
|
|
||
| private suspend fun signWithHdKeyAccount(transactionBytes: ByteArray, hdKey: LocalAccount.HdKey): ByteArray? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LocalAccountSignHelper can be used instead
| val signedTransaction = Encoder.decodeFromMsgPack(signedTransactionBytes, SignedTransaction::class.java) | ||
| signedTransaction.sig?.bytes?.takeIf { it.isNotEmpty() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this into AlgoSdk
|
|
||
| @OptIn(ExperimentalMaterial3Api::class) | ||
| @Composable | ||
| fun BottomSheetContent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be private
| @OptIn(ExperimentalMaterial3Api::class) | ||
| @Suppress("MagicNumber") | ||
| @Composable | ||
| fun BottomSheetHeader( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be private
| val accountAddresses = transactions.transactions | ||
| .flatMap { it.getTransactionsThatNeedsToBeSigned() } | ||
| .map { it.accountAddress } | ||
| .distinct() | ||
|
|
||
| for (address in accountAddresses) { | ||
| val accountType = getAccountType(address) | ||
| if (accountType == AccountType.Joint) { | ||
| displayError(Local(AnnotatedString(R.string.joint_accounts_are_not_supported))) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this logic into its on usecase that checks if a transaction requires a Joint account or not.
| } | ||
| val accountErrorItems = localAccounts | ||
| .mapNotNull { localAccount -> | ||
| val registrationType = getAccountRegistrationType(localAccount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use local account type instead of registration type
| ) | ||
| } | ||
|
|
||
| private fun shouldIncludeAccount( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do this filtering before returning local accounts. Since this feature affects whole app, we shouldn't do individual filter
- Do NOT change data class to sealed interface - Do NOT remove wrapper/delegation functions - Do NOT change filter+map to mapNotNull - Do NOT remove TODO placeholders - Do NOT move data classes or companion objects - Do NOT inject use cases directly when helper exists - Do NOT change existing class dependencies - Do NOT rename existing identifiers without explicit request - Do NOT remove string resources - Do NOT simplify when expressions Also add clarifying comment to TODO in AssetTransferPreviewFragment
- Remove unnecessary getLocalAccount wrapper from LocalAccountSigningHelper - Refactor JointAccountTransactionSignHelper: - Use LocalAccountSigningHelper for signing - Change JointSignResult from data class to sealed interface - Move data classes and companion object to bottom of file - Add TODO comment for extractSignatureFromSignedTransaction - Use mapNotNull instead of filter+map in AccountPreviewProcessor - Remove duplicate TransactionSigner.Joint branch in TransactionSignManager - Update TransactionSignManager to use sealed interface pattern
1. AccountRecoveryTypeSelectionScreen.kt: - Make BottomSheetContent private - Make BottomSheetHeader private - Fix scrolling title - title no longer scrolls with content 2. SwapConfirmationViewModel.kt: - Extract joint account check to IsJointAccountInAddresses use case - Add IsJointAccountInAddressesUseCase implementation - Add DI binding in SwapUiModule 3. AccountPreviewProcessor.kt: - Use local account type instead of registration type - Add getLocalAccountType helper function 4. GroupChoiceWidget.kt: - Replace showNewBadge boolean with badge composable slot - Add GroupChoiceNewBadge composable for reusable badge 5. PeraToolbar.kt: - Remove rightLabel parameter, use endContainer instead - Add PeraToolbarLinkText for link-styled text - Add PeraToolbarTitle for default title text - Add PeraToolbarLargeTitle for large title text - Add PeraToolbarTitleWithSubtitle for title with subtitle - Keep textStyle for backward compatibility
1. TransactionManagerResult - Use sealed Success class: - Convert Success from data class to sealed class - Add Success.SignedTransaction for completed transactions - Add Success.TransactionRequestSigned for joint account requests - Update all usages in TransactionSignManager, TransactionSignBaseFragment, MainActivity 2. GetLocalAccounts - Filter joint accounts at use case level: - GetLocalAccountsUseCase now filters joint accounts when feature toggle is off - GetLocalAccountsFlowUseCase now filters joint accounts when feature toggle is off - Remove redundant shouldIncludeAccount filtering from AccountPreviewProcessor - Joint account filtering is now consistent across the entire app
- Add signWithAlgo25AccountReturnSignature using Sdk.signTransactionReturnSignature - Add signWithHdKeyAccountReturnSignature using existing HD wallet API - Rename signWithAlgo25 -> signWithAlgo25Account - Rename signWithHdKey -> signWithHdKeyAccount - Rename signTransactionSignatureOnly -> signTransactionReturnSignature - Update Go mobile SDK version to 1.0.9 - Clean and organize cursor rules file
- Fix MaxLineLength in AccountPreviewProcessor - Suppress TooManyFunctions in SwapUiModule (pre-existing) - Suppress LongParameterList in TransactionSignManager (pre-existing)
- Split SwapUiModule into SwapUiModule and SwapTrackingModule - Create AccountBalanceProvider to group balance-related use cases - Reduce TransactionSignManager constructor parameters from 10 to 8 - Add cursor rule: never use @Suppress without approval
* multisig/04-misc-updates: Fix detekt issues without @Suppress Fix pre-existing detekt issues Use Go SDK for transaction signing and return signature directly fix: Address remaining PR #514 review comments fix: Address PR #514 review comments fix: Address PR #514 feedback docs: Add refactoring restrictions to prevent unwanted changes
…-support * multisig/05-data-models: Address PR #515 comments for data models Fix detekt issues without @Suppress Fix pre-existing detekt issues Use Go SDK for transaction signing and return signature directly fix: Address remaining PR #514 review comments fix: Address PR #514 review comments fix: Address PR #514 feedback docs: Add refactoring restrictions to prevent unwanted changes
Summary
JointAccountTransactionSignHelperfor proposing sign requestsLocalAccountSigningHelperfor local account signingTransactionSignManagerto handleTransactionSigner.JointExternalTransactionSignManagerfor joint account supportTransactionSigner.Jointcase handling across the appAccountRecoveryTypeSelectionFragmentto ComposeContactIconcomposable andPeraToolbarimprovementsTest Plan