-
Notifications
You must be signed in to change notification settings - Fork 14
feat(deeplink): Add joint account deep link handling and transaction DTOs #513
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/02-data-layer
Are you sure you want to change the base?
Conversation
e196a43 to
72c4117
Compare
- Implement joint account import deep link parser - Add deep link navigation to joint account screens - Add unit tests for deep link builders - Add unit tests for joint account mappers - Add unit tests for repository implementations
b98a876 to
5211943
Compare
…onse - Delete AssetInboxRequestMapperImplTest.kt - Delete AssetInboxRepositoryImplTest.kt - Delete AssetInboxCacheManagerImplTest.kt These tests reference AssetInboxRequestResponse which no longer exists.
| key: Int | ||
| ): ByteArray? { | ||
| return try { | ||
| val xHDWalletAPI = XHDWalletAPIAndroid(seed) |
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.
If Joint account needs to sign multiple transactions, this implementation will create multiple XHDWalletApiAndroid object and this may cause memory exception. If that's not the case, this should be fine since GC cleans it automatically, but if we are using this function in an iteration, we need to change it. Create wallet by using AlgorandBip39WalletProvider then sign the transaction with the same object, instead of creating new one.
| ) | ||
| }.orEmpty() | ||
| return emptyList() | ||
| TODO("Implement the mapper ") |
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.
Just a reminder. Ignore if this is fixed in other PRs
| import com.google.gson.annotations.SerializedName | ||
|
|
||
| internal data class AssetInboxRequestResponse( | ||
| data class ParticipantSignatureRequest( |
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.
Since this is a data model, it should be internal
|
|
||
| import com.google.gson.annotations.SerializedName | ||
|
|
||
| data class ProposeJointSignRequestRequest( |
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.
Since this is a data model, it should be internal
|
|
||
| import com.google.gson.annotations.SerializedName | ||
|
|
||
| data class SearchSignRequestsRequest( |
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.
Since this is a data model, it should be internal
| import com.algorand.wallet.jointaccount.transaction.domain.model.SignRequestStatus | ||
| import javax.inject.Inject | ||
|
|
||
| class JointSignRequestDTOMapper @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.
This mapper in data package and maps domain model. It should implement public interface and then it should be internal.
| ): PeraResult<List<JointSignRequestDTO>> | ||
|
|
||
| companion object { | ||
| const val INJECTION_NAME: String = "jointAccountRepositoryInjectionName" |
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.
There is no need to use injection name if there is only one instance of JointAccountRepository to provide.
| } | ||
|
|
||
| @Provides | ||
| @Singleton |
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 singleton object
| import retrofit2.http.POST | ||
| import retrofit2.http.Path | ||
|
|
||
| interface JointAccountApiService { |
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 internal
| android.util.Log.w( | ||
| "JointAccountRepository", | ||
| "Failed to map sign request: id=${response.id}" | ||
| ) |
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.
If this is unexpected and important, we should use pera error logger instead of console logging
…link Resolves merge conflicts and applies data layer visibility rules: - Restore DTOs (domain models needed by public repository interface) - Make API service internal - Make data layer mappers internal - Make request/response models internal
- Rename JointAccountDTO → JointAccount - Rename CreateJointAccountDTO → CreateJointAccountInput - Rename JointSignRequestDTO → JointSignRequest - Rename ParticipantSignatureDTO → ParticipantSignature - Rename ProposeJointSignRequestDTO → CreateSignRequestInput - Rename SearchSignRequestsDTO → SearchSignRequestsInput - Rename SignRequestTransactionListResponseDTO → AddSignatureInput - Rename SignRequestWithFullSignatureDTO → SignRequestWithFullSignature Also rename related mappers to match new domain model names: - ProposeJointSignRequestDTOMapper → CreateSignRequestInputMapper - SearchSignRequestsDTOMapper → SearchSignRequestsInputMapper - SignRequestTransactionListResponseDTOMapper → AddSignatureInputMapper Update all usages across repository, use cases, and tests. Mock JointAccountDTOMapper in JointSignRequestMapperTest per PR review. Use companion object for test constants per PR review.
Create JointAccountTransactionUseCases.kt containing all use case interfaces (AddJointAccountSignature, GetSignRequestWithSignatures, ProposeJointSignRequest) per PR review feedback. Keep implementation classes in their own files.
Per PR review, use cases that just delegate to repository are simplified: - AddJointAccountSignature: provided directly via DI lambda - ProposeJointSignRequest: provided directly via DI lambda - GetSignRequestWithSignatures: mapping logic moved to repository Changes: - Add getSignRequestWithSignatures() to JointAccountRepository - Move mapping logic to JointAccountRepositoryImpl - Remove searchSignRequests() from public interface (internal only) - Delete use case implementation classes - Delete use case tests (logic now covered by repository) - Update DI module to provide interfaces via lambdas
Follow project rule: "One public class/interface per file"
New rules based on PR review feedback: Data Layer: - Data models (Request/Response) must be internal Domain Layer: - Domain models do NOT use DTO suffix - Use descriptive input/output naming (*Input suffix) UseCase Pattern: - Avoid unnecessary use case implementations - Provide simple use cases via DI lambda - Only create implementation when there's business logic Unit Test Best Practices: - Mock ALL dependencies (don't test multiple classes) - Use companion object for test constants - Use .copy() for test variations
…MapperTest Follow the new rule: use companion object for test constants
- Simple helpers without parameters → companion object - Helpers with default parameters from constants → private function
Summary
JointAccountImportDeepLinkBuilderfor sharing joint accountsJointSignRequestDTO,ProposeJointSignRequestDTO)AddJointAccountSignature,ProposeJointSignRequestuse casesTest Plan