feat(BUX-291): adjust tx recording to BEEF requirements #450

arkadiuszos4chain · 2023-10-30T07:56:15Z

Motivation

We need to modify the synchronization and broadcasting mechanisms to align with BEEF payment requirements. Additionally, we seek greater control over the flow of sending and receiving transactions to facilitate future maintenance and development.

Solution

Implement strategies for recording transactions
Separate business logic from the persistence logic. Please note that this results in some redundant code, which should be improved in the future. However, for the purposes of this PR, I've opted to leave it as is to limit the scope

Pull Request Checklist

📖 I created my PR using provided : CODE_STANDARDS
📖 I have read the short Code of Conduct: CODE_OF_CONDUCT
🏠 I tested my changes locally.
✅ I have provided tests for my changes.
📝 I have used conventional commits.
📗 I have updated any related documentation.
💾 PR was issued based on the Github or Jira issue.

codecov · 2023-10-31T10:51:28Z

Codecov Report

Merging #450 (989dd4c) into rc-beef-v1-payments (9bbbedf) will decrease coverage by 0.29%.
The diff coverage is 49.57%.

Additional details and impacted files

@@                   Coverage Diff                   @@
##           rc-beef-v1-payments     #450      +/-   ##
=======================================================
- Coverage                52.78%   52.50%   -0.29%     
=======================================================
  Files                      101      106       +5     
  Lines                    10855    11153     +298     
=======================================================
+ Hits                      5730     5856     +126     
- Misses                    4674     4828     +154     
- Partials                   451      469      +18

Flag	Coverage Δ
unittests	`52.50% <49.57%> (-0.29%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
model_sync_transactions.go	`70.80% <ø> (+0.25%)`	⬆️
model_transactions.go	`77.89% <100.00%> (-3.58%)`	⬇️
beef_tx_bytes.go	`0.00% <0.00%> (-78.34%)`	⬇️
db_model_transactions.go	`58.78% <0.00%> (-2.93%)`	⬇️
model_merkle_proof.go	`85.18% <82.35%> (+1.62%)`	⬆️
tx_repository.go	`44.69% <50.00%> (+0.18%)`	⬆️
model_draft_transactions.go	`72.62% <55.55%> (-0.22%)`	⬇️
tx_service.go	`62.91% <90.38%> (+15.85%)`	⬆️
utils.go	`11.11% <11.11%> (ø)`
paymail_service_provider.go	`0.00% <0.00%> (ø)`
... and 9 more

... and 4 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bbbedf...989dd4c. Read the comment docs.

wregulski · 2023-11-01T07:51:01Z

model_transactions.go

-				}
-			}
+	if txInfo.MerkleProof != nil {
+		mp := MerkleProof(*txInfo.MerkleProof)


I would create submethod here or sth to do all of MekleRoot related stuff.

I changed it, let me know what do you think about it

wregulski · 2023-11-01T07:55:11Z

record_tx.go

+			XPubKey:        xPubKey,
+		}
+	} else {
+		tx, err := getTransactionByHex(ctx, c, txHex)


For me to make this code more self descriptive it would be nice if in this if/else (especially in else) create a smaller method/function to handle this code and it's name should tell us what it is exactly doing.

I changed it... TBH I don't think it's better now but have no idea how make it better

wregulski · 2023-11-01T10:29:54Z

model_transactions_test.go

@@ -743,7 +757,19 @@ func (ts *EmbeddedDBTestSuite) TestTransaction_Save() {
 		// add the IN transaction
 		transactionIn := newTransaction(testTx2Hex, append(tc.client.DefaultModelOptions(), New())...)
 		require.NotNil(t, transactionIn)
+		// TODO: move this to one place


General comment regarding TODOs/questions left as comments:

First category of TODOs
I would segregate the TODOs into ones you didn't find answers to but are key to closing this shuffle. Then team members could help you investigate.
For example:
https://github.com/BuxOrg/bux/pull/450/files#diff-e28c902ea80bcbe696ef6a0172e5c5cf443846c446de2830367cd36e192da4b0R124

Second category of TODOs
The second category of TODOs are those that we will not currently resolve but do not block the pull request merge.
In this case, I would create a task in Jira that contains the appropriate subtasks associated with the specific problem.

I don't mind leaving the TODOs as they are:
// TODO: BUX-XXX check if we can ommit broadcasting here

TODO you referred to is not mine - I just moved this function from one file to another. I removed all TODOs and questions added by me.

BTW. if you think this function should go back to its previous file let me know.

wregulski · 2023-11-01T10:31:28Z

action_transaction.go

Docs:
I would definitely go here:
https://github.com/bitcoin-sv/bux-docs/tree/main/transactions
and check if we can update current docs.

The most important thing for me is transaction flow diagram.

model_incoming_transactions.go

kuba-4chain

Epic work 💪

…x_service _processInputs()

… requirements

…ication fails

…EF to be more coherent

…rategies

arkadiuszos4chain self-assigned this Oct 30, 2023

mergify bot added the feature Any new significant addition label Oct 30, 2023

arkadiuszos4chain force-pushed the feat-291-change-broadcast-rc branch from 471d187 to b09677a Compare October 30, 2023 20:11

arkadiuszos4chain force-pushed the feat-291-change-broadcast-rc branch 2 times, most recently from 46ff1b7 to 6c5489d Compare October 31, 2023 14:25

arkadiuszos4chain changed the title ~~Feat 291 change broadcast rc~~ feat(BUX-291): adjust tx recording to BEEF requirements Oct 31, 2023

arkadiuszos4chain marked this pull request as ready for review October 31, 2023 15:45

arkadiuszos4chain requested a review from a team as a code owner October 31, 2023 15:45

arkadiuszos4chain requested a review from a team October 31, 2023 15:46

wregulski reviewed Nov 1, 2023

View reviewed changes

arkadiuszos4chain force-pushed the feat-291-change-broadcast-rc branch from 168b2be to 844cc72 Compare November 1, 2023 10:30

wregulski suggested changes Nov 1, 2023

View reviewed changes

wregulski reviewed Nov 1, 2023

View reviewed changes

model_incoming_transactions.go Show resolved Hide resolved

pawellewandowski98 approved these changes Nov 2, 2023

View reviewed changes

kuba-4chain reviewed Nov 2, 2023

View reviewed changes

model_incoming_transactions.go Show resolved Hide resolved

model_incoming_transactions.go Show resolved Hide resolved

arkadiuszos4chain requested a review from sirdeggen November 2, 2023 14:51

kuba-4chain approved these changes Nov 2, 2023

View reviewed changes

arkadiuszos4chain force-pushed the feat-291-change-broadcast-rc branch 2 times, most recently from 3149f56 to 4cafaf3 Compare November 6, 2023 15:50

arkadiuszos4chain added 8 commits November 13, 2023 14:51

feat(BUX-291): implement strategies for tx recording

9f23468

feat(BUX-291): refactorization- move Transaction.processInputs() -> t…

fbf0286

…x_service _processInputs()

feat(BUX-291): remove business logic from DB hooks / adjust BL to new…

69fdc32

… requirements

feat(BUX-291): use new record strategies

52af8c9

feat(BUX-291): fix tests

0d62c78

feat(BUX-291): remove comments

c22938e

feat(BUX-291): remove added TODOs and questions

1dbcc76

feat(BUX-291): adjust to review

c6d3008

arkadiuszos4chain and others added 14 commits November 13, 2023 14:51

feat(BUX-291): outgoing strategy change - do not proceed if P2P notif…

a9bab6d

…ication fails

feat(BUX-291): change sync.BroadcastStatus to Skipped for outgoing BE…

f020d67

…EF to be more coherent

feat(BUX-291): fix infinity log message on synctx record issue

a81856f

feat(BUX-291): fix (shame on me) outgoing BEEF; add logs to record st…

8a0d835

…rategies

feat(BUX-291): prevent saving syncTx hwen outgoing tx is rejected

ad7a67e

feat(BUX-291): make strategies more readable

e4fe87e

tests(BUX-250): fix encoding BEEF - encode correct ver..sion

d4380d5

fix(BUX-250): handle missing paymail error (#465)

70e0f74

tests(BUX-250): save BlokHeight in BUMP

0a32d81

feat(BUX-250): BUMP duplicates support and bump merging refactored

9f25e24

feat(BUX-250): fixing BUMP merging according to BRC - matching roots

61fd134

fix: tests removal, will be added in bux-168

d0e1394

feat(BUX-250): block height added to TuBUMP method

4307026

feat(BUX-250): tests removed

989dd4c

arkadiuszos4chain force-pushed the feat-291-change-broadcast-rc branch from cf413a1 to 989dd4c Compare November 13, 2023 13:51

arkadiuszos4chain changed the base branch from master to rc-beef-v1-payments November 13, 2023 13:51

arkadiuszos4chain merged commit b59ead9 into rc-beef-v1-payments Nov 13, 2023
9 of 10 checks passed

mergify bot deleted the feat-291-change-broadcast-rc branch November 13, 2023 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(BUX-291): adjust tx recording to BEEF requirements #450

feat(BUX-291): adjust tx recording to BEEF requirements #450

arkadiuszos4chain commented Oct 30, 2023 •

edited

Loading

codecov bot commented Oct 31, 2023 •

edited

Loading

wregulski Nov 1, 2023

arkadiuszos4chain Nov 1, 2023

wregulski Nov 1, 2023

arkadiuszos4chain Nov 1, 2023

wregulski Nov 1, 2023 •

edited

Loading

arkadiuszos4chain Nov 1, 2023 •

edited

Loading

wregulski Nov 1, 2023

arkadiuszos4chain Nov 13, 2023

kuba-4chain left a comment

feat(BUX-291): adjust tx recording to BEEF requirements #450

feat(BUX-291): adjust tx recording to BEEF requirements #450

Conversation

arkadiuszos4chain commented Oct 30, 2023 • edited Loading

Motivation

Solution

Pull Request Checklist

codecov bot commented Oct 31, 2023 • edited Loading

Codecov Report

wregulski Nov 1, 2023

Choose a reason for hiding this comment

arkadiuszos4chain Nov 1, 2023

Choose a reason for hiding this comment

wregulski Nov 1, 2023

Choose a reason for hiding this comment

arkadiuszos4chain Nov 1, 2023

Choose a reason for hiding this comment

wregulski Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

General comment regarding TODOs/questions left as comments:

arkadiuszos4chain Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

wregulski Nov 1, 2023

Choose a reason for hiding this comment

arkadiuszos4chain Nov 13, 2023

Choose a reason for hiding this comment

kuba-4chain left a comment

Choose a reason for hiding this comment

arkadiuszos4chain commented Oct 30, 2023 •

edited

Loading

codecov bot commented Oct 31, 2023 •

edited

Loading

wregulski Nov 1, 2023 •

edited

Loading

arkadiuszos4chain Nov 1, 2023 •

edited

Loading