ivy-wallet icon indicating copy to clipboard operation
ivy-wallet copied to clipboard

[Data Layer] Migrate the code to use the new the `TransactionRepository`

Open ILIYANGERMANOV opened this issue 1 year ago • 11 comments

Please confirm the following

What would you like to improve?

  • Implement TransactionRepository for handling data operations
  • Migrate from using TransactionDao and WriteTransactionDao directly to leveraging the repository

Because...

  • To transition away from the outdated data management practices
  • To ensure consistency in the architectural framework
  • To improve safety and simplify the process of making future enhancements

Description

No response

Success Criteria

  • Adequate unit testing coverage
  • Uninterrupted functionality of pre-existing features
  • The application's continuous stability and performance with the newly adopted data layer

ILIYANGERMANOV avatar Feb 14 '24 23:02 ILIYANGERMANOV

Thank you @ILIYANGERMANOV for raising Issue #2963! 🚀 What's next? Read our Contribution Guidelines 📚.

Tagging @ILIYANGERMANOV for review & approval 👀

ivywallet avatar Feb 14 '24 23:02 ivywallet

I'm on it

michalguspiel avatar Feb 15 '24 07:02 michalguspiel

Thank you for your interest @michalguspiel! 🎉 Issue #2963 is assigned to you. You can work on it! ✅

If you don't want to work on it now, please un-assign yourself so other contributors can take it.

Also, make sure to read our Contribution Guidelines.

ivywallet avatar Feb 15 '24 07:02 ivywallet

@ILIYANGERMANOV

I understand that

Implement TransactionRepository for handling data operations part of this task is already done.

Thus this task requires to leverage TransactionRepository in the domain layer instead of directly calling Data Access Objects?

michalguspiel avatar Feb 19 '24 18:02 michalguspiel

@ILIYANGERMANOV

I understand that

Implement TransactionRepository for handling data operations part of this task is already done.

Thus this task requires to leverage TransactionRepository in the domain layer instead of directly calling Data Access Objects?

Yup, correct. It's migration DAOs usages to the repository ones. This also includes using the new domain model

ILIYANGERMANOV avatar Feb 19 '24 18:02 ILIYANGERMANOV

Okay so basically plan for this is to add internal visibility modifier to both TransactionDao and WriteTransactionDao and fix the errors.

What do you mean by new domain model?

michalguspiel avatar Feb 19 '24 18:02 michalguspiel

Okay so basically plan for this is to add internal visibility modifier to both TransactionDao and WriteTransactionDao and fix the errors.

What do you mean by new domain model?

This. When you fix the errors you'll need to pass Transaction instance and not TransactionEntity. But it's exactly what you said

ILIYANGERMANOV avatar Feb 19 '24 18:02 ILIYANGERMANOV

@ILIYANGERMANOV

Starting to work with this, I'm realizing how huge of a task it is. It's going to take weeks if not months, considering the amount of my free time I have for this. By the time I'm done with this, I'll need to resolve tons of conflicts which will extend this work even further.

When migrating to the new TransactionRepository it doesn't quite make sense to map the results in the Presentation layer back to the legacy Domain model. Because of that a lot of logic needs to be refactored.

Have you any idea how to split this task to smaller chunks?

For instance: instead of Migrate from using TransactionDao and WriteTransactionDao directly to leveraging the repository We could do: Migrate from using TransactionDao directly to leveraging the TransactionRepository Migrate from using Write TransactionDao to leveraging the TransactionRepository

WDYT

michalguspiel avatar Feb 20 '24 05:02 michalguspiel

@michalguspiel yes your suggestion is good 👍 You can split it to even smaller, migrate one or a few usages -> submit a PR, merge it. This way it would be easier to review

ILIYANGERMANOV avatar Feb 20 '24 06:02 ILIYANGERMANOV

What do we do about TransactionHistoryItem? I got to the point where passing history: List<TransactionHistoryItem> to transactions composable creates a big problem for refactoring.

TransactionHistoryItem is deprecated but TransactionHistoryDateDivider is not. Legacy Transaction and TransactionHistoryDateDivider both inherit from TransactionHistoryItem. How do we want to fix this in regard to new Transaction model?

I could have another PR ready soon if I would skip this problem for now by creating duplicate TransactionCard composable that uses legacy Transaction but I'd like to somehow mitigate that.

Any suggestions?

michalguspiel avatar Feb 26 '24 04:02 michalguspiel

I could have another PR ready soon if I would skip this problem for now by creating duplicate TransactionCard composable that uses legacy Transaction but I'd like to somehow mitigate that.

@michalguspiel just duplicate it - that's a good strategy!

ILIYANGERMANOV avatar Feb 26 '24 09:02 ILIYANGERMANOV