Add memo field to withdrawal transaction #4229
Conversation
ghost
marked this pull request as draft
|
I notice you have added a new storage class for transactions. There is already the bitcoinj Transaction which stores the transactions and has a memo field. What was the reasons for not using that? I suppose those transactions would be lost in an SPV resync. Just feels like quite a bit of extra work to store the memo field separately like this. |
ghost
changed the title
|
Thank you very much for direction. I have updated the code in this PR: For now, I'll keep commits (to allow anyone to see the hisstory in this PR) but I will of course squash it before the merge. |
ghost
marked this pull request as ready for review
| @@ -164,7 +163,8 @@ private WithdrawalView(BtcWalletService walletService, | |||
| @Named(FormattingUtils.BTC_FORMATTER_KEY) CoinFormatter formatter, | |||
| Preferences preferences, | |||
| BtcAddressValidator btcAddressValidator, | |||
| WalletPasswordWindow walletPasswordWindow) { | |||
| WalletPasswordWindow walletPasswordWindow | |||
| ) { | |||
There was a problem hiding this comment.
This is not customary. Not sure if it's in the style guide, but better keep it in line with other code and not line break before the parenthesis.
There was a problem hiding this comment.
👍 this looks like some forgotten change after reverting my previous changes.
Maybe question/suggestion - is there any good automated style check (ideally with fixer) to solve issues like this to save some time? I am not really experienced in Java ecosystem, but I believe there is something like https://eslint.org/ but for Java.
- "Memo" field is modeled as property of the new object Transaction which is stored in persitant storage. - Transaction object is modeled in a way that allows extension in the furure for more persisted attributes.
…ansaction object)
|
@sqrrm About the dark mode tooltip. I am not sure if its a bug in my PR. As I understand it, I am using standard From this PR: Am I missing something? If not, I suggest to create a special Issue for this as bug in the |
Will solve #1387
This is a Draft intended for early review because it is my first contribution during onboarding.Preview: