refactor `account/watcher` and add mocks/tests #316

positiveblue · 2021-11-21T01:02:37Z

Make account/manager take an interface for the watcher instead of a explicit type.
Split the watcher in two components:
- Controller: initialize/delete logic + dispatch handlers
- Watcher: watcher state + handle logic
Add tests for controller + watcher

guggero

Very nice to see the new mocks in action. The refactor looks pretty good to me, just a few suggestions for the naming and how to simplify it even more.

account/watcher/watcher.go

account/watcher/controller.go

account/watcher/watcher.go

account/watcher/controller_test.go

guggero

Looking good! Way better with the EventHandler interface IMO. See my comment about getting rid of the Handlers struct as well.
After that we're good to go 🎉

account/watcher/controller.go

guggero · 2021-12-02T09:08:20Z

account/watcher/interfaces.go

+
+// Controller is the interface used by other components to communicate with the
+// watcher.
+


nit: extra newline.

Ah, it's removed in a later commit. Can move it here.

guggero · 2021-12-02T09:14:47Z

account/manager.go

@@ -153,13 +153,17 @@ func NewManager(cfg *ManagerConfig) *Manager {
 		quit: make(chan struct{}),
 	}

-	m.watcherCtrl = watcher.NewController(&watcher.Config{
-		ChainNotifier:       cfg.ChainNotifier,
+	eventHandler := watcher.NewEventHandler(&watcher.Handlers{


I think we should just export those handle* methods on the manager and just pass m here instead of adding this wrapper struct.

account/watcher/watcher.go

positiveblue · 2021-12-02T17:17:10Z

@guggero I don't know why I cannot reply under every comment so I will reply to all of them here:

one downside of having a non-exported implementation

yes, I would avoid both: return a non exported struct type and return an interface. However, given that this is only used in account/manager having it non exported is not a big problem.

export those handle* methods on the manager and just pass m here instead of adding this wrapper struct.

(Y)

guggero

Great initiative on this 💯 Some final nits, otherwise LGTM 🎉

account/watcher/controller.go

account/manager.go

account/watcher/controller.go

bhandras

Nice refactors, and tests! My only major comment is about the expiry watcher, other than that looks good!

bhandras · 2021-12-03T14:16:00Z

account/watcher/watcher.go

+}
+
+// OverdueExpirations handles the expirations for the given block.
+func (w *expiryWatcher) OverdueExpirations(blockHeight uint32) {


I think a better design could be to have a priority queue of accounts where the priority is defined by the block height. This would allow us to expire everything before and at the specified block height.

Or alternatively make newblock/expiry atomic.

bhandras · 2021-12-03T14:18:49Z

account/watcher/watcher.go

+			continue
+		}
+
+		err := w.handlers.HandleAccountExpiry(


Do we need to lock for HandleAccountExpiry? An alternative could be to collect expiring accounts in the lock then expire them outside the lock.

bhandras · 2021-12-03T14:22:22Z

account/watcher/watcher.go

+
+// NewBlock updates the current bestHeight.
+func (w *expiryWatcher) NewBlock(bestHeight uint32) {
+	w.bestHeight = bestHeight


Since expiry happens when the height is changed, it'd be possible to simplify the interface a bit and do the height change and the expiry handling atomically under the same lock. By only expiring the current block if two blocks come right after each other in succession we may miss an expiration (almost no chance in practice but still).

account/watcher/controller_test.go

account/watcher/watcher_test.go

positiveblue · 2021-12-07T06:45:17Z

Feedback updated 👍

For some of the question:

better design could be to have a priority queue of accounts where the priority is defined by the block height

Totally agree. I think it is something we can add later though. Do you know any good implementation that allows update the priority of the elements in it?

By only expiring the current block if two blocks come right after each other in succession we may miss an expiration (almost no chance in practice but still).

Yes I thought about this but it cannot happen because the controller was calling the two funcs (new block + overdue expirations) before being able to receive another block (iterate the select). I made them atomic, better put it together so we do not get confused and use the API wrong

carlaKC

Just did a high level pass since there are already 2 reviewers. Great work on interface refactors + mock :)

carlaKC · 2021-12-07T08:25:57Z

account/watcher/controller.go

-// that is confirmation or spend.
-type Watcher struct {
+// controller implements the Controller interface
+type controller struct {


Add compile time assertion that we satisfy the controller interface?
var _ Controller = (*controller)(nil)

account/watcher/controller.go

carlaKC · 2021-12-07T08:28:38Z

account/watcher/interfaces.go

+	HandleAccountExpiry(*btcec.PublicKey, uint32) error
+}
+
+// ExpiryWatcher is the interface for the component in charge of the accounts' expiration.


nit: wrap at 80?
These look a bit over

carlaKC · 2021-12-07T08:31:07Z

account/watcher/controller_test.go

+	expectedErr string
+}{{
+	name: "Watch account spend happy path",
+	// TODO (positiveblue): add tests for `cancel` logic


happening in this PR or a follow up?

bhandras

LGTM 💯

bhandras · 2021-12-07T09:15:48Z

account/watcher/controller.go

-			}
-
-			delete(expirationsPerHeight, bestHeight)
+			c.watcher.NewBlock(uint32(newBlock))


One thing to keep in mind is that if HandleAccountExpiry blocks for some time, then the blockChan might get clogged.

Use the new watcher interface in the account manager instead of of a specific struct type.

Split watcher logic in three pices: - Controller: API + message dispatching - ExpiryWatcher: handle account expirations - EventHandler: implementation for each handler

positiveblue force-pushed the watcher-interface-v3 branch 2 times, most recently from 5c32556 to c17d06b Compare November 21, 2021 01:21

guggero self-requested a review November 22, 2021 16:09

guggero reviewed Nov 29, 2021

View reviewed changes

positiveblue force-pushed the watcher-interface-v3 branch from c17d06b to 1727f64 Compare December 2, 2021 06:07

positiveblue requested review from guggero, bhandras and carlaKC December 2, 2021 06:08

positiveblue force-pushed the watcher-interface-v3 branch from 1727f64 to 6d47746 Compare December 2, 2021 06:13

guggero requested changes Dec 2, 2021

View reviewed changes

positiveblue force-pushed the watcher-interface-v3 branch from 6d47746 to ad06024 Compare December 3, 2021 01:07

guggero approved these changes Dec 3, 2021

View reviewed changes

account/watcher/controller.go Show resolved Hide resolved

account/manager.go Show resolved Hide resolved

account/watcher/controller.go Show resolved Hide resolved

bhandras reviewed Dec 3, 2021

View reviewed changes

positiveblue force-pushed the watcher-interface-v3 branch from ad06024 to 9a8fa5e Compare December 7, 2021 06:53

positiveblue requested a review from bhandras December 7, 2021 06:53

positiveblue force-pushed the watcher-interface-v3 branch from 9a8fa5e to f45fa65 Compare December 7, 2021 07:13

carlaKC reviewed Dec 7, 2021

View reviewed changes

bhandras approved these changes Dec 7, 2021

View reviewed changes

bhandras reviewed Dec 7, 2021

View reviewed changes

positiveblue added 6 commits December 7, 2021 11:58

account/watcher: add Controller interface

a34790e

Use the new watcher interface in the account manager instead of of a specific struct type.

account/watcher: add ExpiryWatcher and EventHandler interfaces

abd603d

Split watcher logic in three pices: - Controller: API + message dispatching - ExpiryWatcher: handle account expirations - EventHandler: implementation for each handler

account/watcher: delete expiryReq

7b53791

mocks: add account/watcher mocks

7394985

account/watcher: add controller tests

17e77c3

account/watcher: add ExpiryWatcher tests

1856f79

positiveblue force-pushed the watcher-interface-v3 branch from f45fa65 to 1856f79 Compare December 7, 2021 20:28

guggero merged commit 6884748 into master Dec 8, 2021

guggero deleted the watcher-interface-v3 branch December 8, 2021 15:13

guggero mentioned this pull request May 5, 2022

Watcher interface v2 #362

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor `account/watcher` and add mocks/tests #316

refactor `account/watcher` and add mocks/tests #316

positiveblue commented Nov 21, 2021

guggero left a comment

guggero left a comment

guggero Dec 2, 2021

guggero Dec 2, 2021

guggero Dec 2, 2021

positiveblue commented Dec 2, 2021 •

edited

Loading

guggero left a comment

bhandras left a comment

bhandras Dec 3, 2021

bhandras Dec 3, 2021

bhandras Dec 3, 2021

bhandras Dec 3, 2021

positiveblue commented Dec 7, 2021

carlaKC left a comment

carlaKC Dec 7, 2021

carlaKC Dec 7, 2021

carlaKC Dec 7, 2021

bhandras left a comment

bhandras Dec 7, 2021


		// Controller is the interface used by other components to communicate with the
		// watcher.

refactor account/watcher and add mocks/tests #316

refactor account/watcher and add mocks/tests #316

Conversation

positiveblue commented Nov 21, 2021

guggero left a comment

Choose a reason for hiding this comment

guggero left a 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

positiveblue commented Dec 2, 2021 • edited Loading

guggero left a comment

Choose a reason for hiding this comment

bhandras left a 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

positiveblue commented Dec 7, 2021

carlaKC left a 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

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor `account/watcher` and add mocks/tests #316

refactor `account/watcher` and add mocks/tests #316

positiveblue commented Dec 2, 2021 •

edited

Loading