-
Notifications
You must be signed in to change notification settings - Fork 14
feat(data): Implement joint account data layer #512
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/01-resources
Are you sure you want to change the base?
Conversation
ee276ea to
1e4e4e2
Compare
- Add JointAccountRepository with API integration - Add JointSignRequest API use cases - Add JointAccountInbox API use cases - Add domain models for joint accounts (LocalAccount.Joint) - Add DTOs for API communication - Implement account type detection for joint accounts
e196a43 to
72c4117
Compare
| data object Joint : AccountType | ||
|
|
||
| companion object { | ||
| fun AccountType.canSignTransaction(): Boolean { |
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.
Seems like it is time to move this function in AccountType interface, and let subclasses to implement
| @ColumnInfo("algo_address") | ||
| val algoAddress: String, | ||
| @ColumnInfo("participant_addresses") | ||
| val participantAddresses: String, |
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 we consider creating new table for participant addresses? Right now we won't be able to query. This will cause us to fetch every joint account. For example displaying error when user wants to remove one of the participant accounts from the device etc.
| operator fun invoke(localAccount: LocalAccount.Joint): JointEntity | ||
| } | ||
|
|
||
| internal class JointEntityMapperImpl @Inject constructor( |
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 implementation class into new file
| import com.algorand.wallet.jointaccount.creation.domain.model.CreateJointAccountDTO | ||
| import javax.inject.Inject | ||
|
|
||
| class CreateJointAccountDTOMapper @Inject constructor() { |
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 make this internal and create an interface for it.
| import com.algorand.wallet.jointaccount.creation.domain.model.JointAccountDTO | ||
| import javax.inject.Inject | ||
|
|
||
| class JointAccountDTOMapper @Inject constructor() { |
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 make this internal and create an interface for it.
| import com.algorand.wallet.account.local.domain.usecase.GetLocalAccount | ||
| import javax.inject.Inject | ||
|
|
||
| internal class GetJointAccountUseCase @Inject constructor( |
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 remove this usecase and use joint account repository to return local account object
| import com.algorand.wallet.account.local.domain.usecase.GetLocalAccount | ||
| import javax.inject.Inject | ||
|
|
||
| internal class GetJointAccountParticipantCountUseCase @Inject constructor( |
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.
Can be removed and handled by repository
|
|
||
| package com.algorand.wallet.jointaccount.creation.domain.model | ||
|
|
||
| data class JointAccountDTO( |
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 remove DTO
|
|
||
| package com.algorand.wallet.jointaccount.creation.domain.model | ||
|
|
||
| data class CreateJointAccountDTO( |
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 remove DTO
| operator fun invoke(entity: JointEntity): LocalAccount.Joint | ||
| } | ||
|
|
||
| internal class JointMapperImpl @Inject constructor( |
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 should move this into its own file
Per PR review comments: - Split JointEntityMapper interface and impl into separate files - Split JointMapper interface and impl into separate files - Make CreateJointAccountDTOMapper internal with interface+impl pattern - Make JointAccountDTOMapper internal with interface+impl pattern - Make CreateJointAccountRequest internal - Make JointAccountResponse internal - Add DI bindings for new mapper interfaces - Update tests to use Impl classes
Per PR review: Move canSignTransaction() function from companion object to interface method with subclass implementations. - Each AccountType subtype now overrides canSignTransaction() - Removed companion object with extension function - Updated all call sites to remove companion import
Per PR review comments: - Remove GetJointAccount/GetJointAccountUseCase (use repository directly) - Remove GetJointAccountParticipantCount/GetJointAccountParticipantCountUseCase - Remove JointAccountDTO and CreateJointAccountDTO - Remove JointAccountDTOMapper and CreateJointAccountDTOMapper - Remove related tests - Update DI module
| version = dto.version | ||
| ) | ||
| } | ||
| internal interface CreateJointAccountDTOMapper { |
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 rename mapper same as class it maps
| import com.algorand.wallet.jointaccount.creation.domain.model.CreateJointAccountDTO | ||
| import javax.inject.Inject | ||
|
|
||
| internal class CreateJointAccountDTOMapperImpl @Inject constructor() : CreateJointAccountDTOMapper { |
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 rename mapper same as class it maps
| ) | ||
| } | ||
| } | ||
| internal interface JointAccountDTOMapper { |
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 rename mapper same as class it maps
| import com.algorand.wallet.jointaccount.creation.domain.model.JointAccountDTO | ||
| import javax.inject.Inject | ||
|
|
||
| internal class JointAccountDTOMapperImpl @Inject constructor() : JointAccountDTOMapper { |
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 rename mapper same as class it maps
| ): GetJointAccountProposerAddress = useCase | ||
|
|
||
| @Provides | ||
| fun provideCreateJointAccountDTOMapper( |
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 rename provider same as class provides
| ): CreateJointAccountDTOMapper = impl | ||
|
|
||
| @Provides | ||
| fun provideJointAccountDTOMapper( |
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 rename provider same as class provides
Summary
LocalAccount.Jointdomain model with participant addresses and thresholdJointDaoandJointEntityfor local database storageJointAccountRepositoryinterface and implementationGetJointAccount,GetJointAccountProposerAddressuse casesTest Plan
./gradlew :common-sdk:testDebugUnitTest