feat(SPV-1535) handle ARC callback #935

chris-4chain · 2025-02-27T08:46:01Z

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.

github-actions · 2025-02-27T08:46:16Z

Manual Tests

ℹ️ Remember to ask team members to perform manual tests and to assign tested label after testing.

codecov-commenter · 2025-02-27T08:48:56Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 2.76498% with 211 lines in your changes missing coverage. Please review.

Project coverage is 35.30%. Comparing base (1c04eb0) to head (80602a5).

Files with missing lines	Patch %	Lines
engine/v2/transaction/txsync/txsync_service.go	0.00%	48 Missing ⚠️
engine/v2/database/repository/transactions.go	0.00%	41 Missing ⚠️
engine/chain/internal/arc/callback.go	0.00%	34 Missing ⚠️
engine/testabilities/assert_engine.go	0.00%	33 Missing ⚠️
engine/tester/http_sniffer.go	0.00%	13 Missing ⚠️
...egrationtests/testabilities/ability_arc_actions.go	0.00%	10 Missing ⚠️
engine/testabilities/fixture_configopts.go	0.00%	6 Missing ⚠️
engine/testabilities/fixture_engine.go	0.00%	6 Missing ⚠️
.../integrationtests/testabilities/ability_actions.go	0.00%	5 Missing ⚠️
engine/tester/paymailmock/mock_paymail_client.go	0.00%	4 Missing ⚠️
... and 5 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #935      +/-   ##
==========================================
- Coverage   35.63%   35.30%   -0.33%     
==========================================
  Files         424      427       +3     
  Lines       20035    20236     +201     
==========================================
+ Hits         7139     7145       +6     
- Misses      12299    12494     +195     
  Partials      597      597

Flag	Coverage Δ
unittests	`35.30% <2.76%> (-0.33%)`	⬇️

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

Files with missing lines	Coverage Δ
...al/integrationtests/testabilities/assert_actors.go	`0.00% <ø> (ø)`
engine/chain/models/tx_info.go	`0.00% <ø> (ø)`
engine/client_internal.go	`70.72% <100.00%> (+0.67%)`	⬆️
engine/v2/database/tracked_transaction.go	`0.00% <ø> (ø)`
initializer/config_to_options.go	`0.00% <ø> (ø)`
...ions/testabilities/assert_spvwallet_application.go	`0.00% <0.00%> (ø)`
...egrationtests/testabilities/fixture_user_actors.go	`0.00% <0.00%> (ø)`
config/utils.go	`14.28% <0.00%> (-2.39%)`	⬇️
engine/client.go	`56.32% <33.33%> (-0.45%)`	⬇️
server/server.go	`5.95% <0.00%> (-0.23%)`	⬇️
... and 10 more

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 1c04eb0...80602a5. Read the comment docs.

mgosek-4chain · 2025-02-27T11:58:34Z

actions/v2/internal/integrationtests/handle_arc_callback_test.go

+		},
+		"On SentToNetwork do nothing": {
+			txInfo:       minimalTxInfo(chainmodels.SentToNetwork),
+			expectStatus: "BROADCASTED",


Minor suggestion: Would it make sense to group these statuses into constants rather than writing them directly?

Actually, there is such type already defined - I'll adjust this :)

mgosek-4chain · 2025-02-27T13:34:37Z

engine/v2/database/repository/transactions.go

+		Model(&database.TrackedTransaction{}).
+		WithContext(ctx).
+		Where("id = ?", txID).
+		Update("tx_status", status).Error


As I understand it, at this stage of development we are not considering implementing concurrency control mechanisms to avoid stale data? 🤔

Don't understand - could you explain what you're thinking about?

I think that he is saying that you can override tx_status that was already set by different process/routine

dorzepowski · 2025-02-28T08:52:26Z

engine/chain/internal/arc/callback.go

+const bearerSchema = "Bearer "
+
+// RegisterCallback registers the ARC callback handler and sets up final URL sent to ARC during broadcast.
+func (s *Service) RegisterCallback(handler chainmodels.TXInfoHandler, router *gin.Engine) {


Hmmm... imho it should be done in a standard way (like the one for swagger v2)
It's counter intuitive to search for that thing in arc service

dorzepowski · 2025-02-28T08:54:30Z

engine/chain/internal/arc/callback.go

+	hostURL, err := url.Parse(s.arcCfg.Callback.URL)
+	if err != nil {
+		panic(spverrors.Wrapf(err, "failed to parse ARC callback URL: %s", s.arcCfg.Callback.URL))
+	}
+
+	hostURL.Path = callbackPath
+	s.arcCfg.Callback.URL = hostURL.String()


maybe Callback config could have a method for getting callback path and full callback url

dorzepowski · 2025-02-28T09:02:12Z

engine/v2/transaction/txsync/txsync_service.go

+	if err != nil {
+		return spverrors.Wrapf(err, "failed to parse merkle path for transaction %s", txInfo.TxID)
+	}
+


consider adding a check for merklepath (or maybe it will be checked during creation of BEEF hex - worth checking):

Suggested change

_, err = bump.ComputeRootHex(lo.ToPtr(txInfo.TxID))

if err != nil {

return spverrors.Wrapf(err, "failed to validate merkle path %s for transaction %s", txInfo.MerklePath, txInfo.TxID)

}

This will prevent updating merkle path to some invalid value.

Or maybe if error occurs we should store info about this 🤔

The only problem here is that, it is taking some time to calculate this root - but actually we're doing the same for each and every beef

dorzepowski · 2025-02-28T09:12:36Z

engine/v2/transaction/txsync/txsync_service.go

+		return spverrors.Wrapf(err, "failed to get BEEF hex for transaction %s", txInfo.TxID)
+	}
+
+	err = s.transactionsRepo.SetAsMined(ctx, txInfo.TxID, txInfo.BlockHash, txInfo.BlockHeight, beefHex)


Sorry but I don't like this approach, because it is forceing us to move a lot of logic into database layer, which doens't allow to test anything without running the integration tests.

I would suggest to:

get domain transaction from repository

check if last update is older then TxInfo timestamp

if not then omit

call set problematic if it's problematic

call tx.Mined(blockhash, merklepath)

in set merkle path it should mark it as mined

set block height from merklepath

set block hash from block hash

build beef with merkle path

remove raw tx

save tx

dorzepowski · 2025-02-28T09:20:39Z

engine/v2/database/repository/transactions.go

+			"block_height": blockHeight,
+			"tx_status":    txmodels.TxStatusMined,
+			"beef_hex":     beefHex,
+		}).Error


we should clear also connection with SourceTxInputs because those entries are not needed anymore.

dorzepowski · 2025-02-28T09:33:43Z

actions/v2/internal/integrationtests/handle_arc_callback_test.go

+
+			// when:
+			test.txInfo.TxID = receiveTxID
+			when.ARC().ReceivesCallback(test.txInfo)


It should be rather 🙈 :

Suggested change

when.ARC().ReceivesCallback(test.txInfo)

when.ARC().SendsCallback(test.txInfo)

dorzepowski · 2025-02-28T09:35:24Z

actions/v2/internal/integrationtests/handle_arc_callback_test.go

+				WithTxID(receiveTxID).
+				WithTxStatus(test.expectStatus)
+
+			// TODO: Assert for tx block height/hash and BEEF after Searching Transactions is implemented


Maybe we should actually add those information to operations endpoint - because it seems like a common thing that we display in wallets (and use for whatsonchain links)

dorzepowski · 2025-02-28T09:38:35Z

actions/v2/internal/integrationtests/handle_arc_callback_test.go

+		t.Run(name, func(t *testing.T) {
+			// given:
+			given, when, then := testabilities.New(t)
+			cleanup := given.StartedSPVWalletV2(testengine.WithARCCallback("https://example.com", testabilities.ARCCallbackToken))


Why don't we make those configs default?

dorzepowski · 2025-02-28T09:39:54Z

actions/v2/internal/integrationtests/handle_arc_callback_test.go

@@ -0,0 +1,101 @@
+package integrationtests


Why is it in integrations tests?

It should be rather test next to the http handler,
You probably could use a Faucet().TopUp() then and make a different ARC callbacks for different statuses.

dorzepowski · 2025-02-28T09:49:36Z

actions/v2/internal/integrationtests/testabilities/ability_arc_actions.go

+
+type arcActions struct {
+	t       testing.TB
+	fixture *fixture


you have access to SPVWalletApplicationFixture,
maybe it could return config, and you could take auth token from config 🤔

chris-4chain added 7 commits February 26, 2025 12:56

feat(SPV-1555): arc callback test

ac0a1d4

feat(SPV-1535): register arc callback

8ee89e7

feat(SPV-1535): test with broadcast details assertions

bea9266

feat(SPV-1535): handle callback with problematic statuses

7fde38a

feat(SPV-1535): set as mined (without BUMP/BEEF yet)

17bc2be

feat(SPV-1535): set as mined with BUMP

cf71821

feat(SPV-1535): lint

e7068e9

chris-4chain requested a review from a team as a code owner February 27, 2025 08:46

chris-4chain added 2 commits February 27, 2025 09:51

feat(SPV-1535): rename to ReceivesCallback

5c6e77a

Merge branch 'main' into feat/SPV-1535-arc-callback

13da21c

wregulski approved these changes Feb 27, 2025

View reviewed changes

mgosek-4chain approved these changes Feb 27, 2025

View reviewed changes

chris-4chain added 2 commits February 28, 2025 06:14

Merge branch 'main' into feat/SPV-1535-arc-callback

8f547ad

Merge branch 'main' into feat/SPV-1535-arc-callback

80602a5

dorzepowski requested changes Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(SPV-1535) handle ARC callback #935

feat(SPV-1535) handle ARC callback #935

chris-4chain commented Feb 27, 2025

github-actions bot commented Feb 27, 2025

codecov-commenter commented Feb 27, 2025 •

edited

Loading

mgosek-4chain Feb 27, 2025

chris-4chain Feb 28, 2025

mgosek-4chain Feb 27, 2025

chris-4chain Feb 28, 2025

dorzepowski Feb 28, 2025

dorzepowski Feb 28, 2025

dorzepowski Feb 28, 2025

dorzepowski Feb 28, 2025

dorzepowski Feb 28, 2025

dorzepowski Feb 28, 2025

dorzepowski Feb 28, 2025

dorzepowski Feb 28, 2025

dorzepowski Feb 28, 2025

dorzepowski Feb 28, 2025

dorzepowski Feb 28, 2025

+	_, err = bump.ComputeRootHex(lo.ToPtr(txInfo.TxID))
+	if err != nil {
+		return spverrors.Wrapf(err, "failed to validate merkle path %s for transaction %s", txInfo.MerklePath, txInfo.TxID)
+	}

	when.ARC().ReceivesCallback(test.txInfo)
	when.ARC().SendsCallback(test.txInfo)

feat(SPV-1535) handle ARC callback #935

Are you sure you want to change the base?

feat(SPV-1535) handle ARC callback #935

Conversation

chris-4chain commented Feb 27, 2025

Pull Request Checklist

github-actions bot commented Feb 27, 2025

Manual Tests

codecov-commenter commented Feb 27, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 27, 2025 •

edited

Loading