-
Notifications
You must be signed in to change notification settings - Fork 14
feat(inbox): Add inbox data layer and cache management #515
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/04-misc-updates
Are you sure you want to change the base?
Conversation
6a84b56 to
8602ae8
Compare
014714a to
c3e316e
Compare
8602ae8 to
cfd09c6
Compare
c3e316e to
6b1b4d5
Compare
- Add inbox data models for joint account notifications - Add sign request transaction list models - Add participant signature DTOs - Implement inbox cache manager for joint accounts - Add inbox item mappers and transformers
cfd09c6 to
252f8ea
Compare
6b1b4d5 to
a997278
Compare
Resolve merge conflicts: - JointAccountRepository: combine new method signatures with inbox methods - JointAccountRepositoryImpl: add inbox methods with new mapper names - JointAccountModule: combine inbox service with new mapper imports - DefaultAccountDetailAccountsItemProcessor: fix import path - InboxSearchDTOMapper: update to use JointSignRequestMapper - InboxMessagesDTO: update to use JointAccount and JointSignRequest - Make data layer classes internal (InboxSearchDTOMapper, InboxApiService, data models)
Rename domain models to follow naming conventions: - InboxSearchDTO -> InboxSearchInput - InboxMessagesDTO -> InboxMessages - AssetInboxDTO -> AssetInbox Update all references in: - JointAccountRepository and impl - InboxRepository and impl - InboxSearchDTOMapper (method names updated) - InboxCacheManagerImpl - HasInboxItemsForAddressUseCase - InboxUseCases interfaces - Test files
…dressUseCaseTest Replace AssetInboxDTO/JointAccountDTO with AssetInbox/JointAccount to match the actual domain model class names on this branch.
| import kotlinx.coroutines.flow.MutableStateFlow | ||
| import kotlinx.coroutines.flow.asStateFlow | ||
|
|
||
| internal class InboxRepositoryImpl : InboxRepository { |
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 is not a repository, this is in-memory cache. Please check InMemoryCacheProvider.
|
|
||
| @Provides | ||
| @Singleton | ||
| fun provideAssetInboxRepository(inboxRepository: InboxRepository): AssetInboxRepository { |
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 make this singleton if you pass cache in constructor -which is the one should be singleton-
If you pass in memory cache instead of this repository, then repository is the one needs to be singleton
| @Provides | ||
| fun provideRefreshInboxCache( | ||
| inboxCacheManager: InboxCacheManager | ||
| ): RefreshInboxCache = RefreshInboxCache { inboxCacheManager.refreshCache() } |
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.
Wrong usage. This is an implementation of a usecase, it should pass function instead of calling it
RefreshInboxCache(inboxCacheManager::refreshCache)
| import com.algorand.wallet.jointaccount.transaction.data.mapper.JointSignRequestMapper | ||
| import javax.inject.Inject | ||
|
|
||
| internal class InboxSearchDTOMapper @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.
Lets create and interface and remove DTO suffix
| suspend fun getInboxMessages( | ||
| deviceId: Long, | ||
| inboxSearchInput: InboxSearchInput | ||
| ): PeraResult<InboxMessages> | ||
|
|
||
| suspend fun deleteInboxJointInvitationNotification( | ||
| deviceId: Long, | ||
| jointAddress: 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.
These two are belong to inbox feature. We should move them to inbox. JointAccount has nothing to do with getting inbox messages and deleting notifications
* 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
- Rename InboxRepository to InboxInMemoryCache using InMemoryCacheProvider - Remove @singleton from AssetInboxModule where cache is passed in constructor - Use method references instead of lambdas in DI modules - Rename InboxSearchDTOMapper to InboxSearchMapper (interface + Impl) - Move getInboxMessages and deleteNotification from JointAccountRepository to InboxApiRepository - Add rules to prevent these issues in future
…-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
inbox/assetpackageInboxRepositoryandInboxCacheManagerInboxMessagesDTOandInboxSearchDTOmodelsHasInboxItemsForAddressUseCasefor badge indicatorsTest Plan