peer: more interfaces, refactor unit tests #5067

Crypt-iQ · 2021-03-03T22:59:46Z

By adding more interfaces, we reduce dependencies and can test the peer package without accessing internal members. Only the last commit is diff-heavy.

cfromknecht

nice job on this refactor! main comment is re: whether or not we can reuse the mock connection in wtmock, would be great if we can start to unify all of our mocks to reduce code duplication

peer/test_utils.go

cfromknecht · 2021-04-07T23:45:09Z

@Crypt-iQ looks like build is failing after the rebase, need to adjust some interfaces due to #5033

Crypt-iQ · 2021-04-09T11:49:48Z

@cfromknecht fixed

yyforyongyu

This is great work! Def will make our testing more maintainable!

Still reading the relevant code to understand the change. One question is, should the interfaces live in peer or should they live in their "original" package, such as MessageSwitch in htlcswitch or in peer? If all the interfaces live in peer, it has the advantage that the interfaces are abstracted as exactly what the peer needs, which imo is also the downside as it might not fit the abstraction needed by other packages.

peer/brontide.go

yyforyongyu · 2021-07-05T01:10:45Z

peer/brontide.go

+
+// MakeChannelSwitch initializes a ChannelSwitch given a raw *htlcswitch.Switch
+// pointer.
+func MakeChannelSwitch(innerSwitch *htlcswitch.Switch) *ChannelSwitch {


nit: use NewChannelSwitch since it's commonly used style in this repo.

peer/brontide.go

yyforyongyu · 2021-07-05T01:21:55Z

peer/interfaces.go

+
+// MessageSwitch is an interface that manages setup, retrieval, and shutdown of
+// MessageLink implementations.
+type MessageSwitch interface {


Looks like htlcswitch.Switch already satisfied most of the functions here. I wonder about the necessity to create the channelSwitch here.

The motivation is to make peer not rely on htlcswitch.Switch, so instead use a ChannelSwitch that can be replaced with a mock. Otherwise you need to provide a *htlcswitch.Switch when testing

channelSwitch needs to wrap GetLink to return MessageLink as *htlcswitch.Switch returns ChannelLink

I don't think you could get over *htlcswitch.Switch since channelSwitch needs it. This design pattern is weird. You have a concrete type defined in another package, and you have an interface defined in this package, then you create a concrete struct that implements the interface, which in the end is the concrete type living in another package.

If we want to be able to replace *htlcswitch.Switch in peer tests (which IMO is worth it, as it does not belong in the peer unit tests):

Need an interface to pass into the peer config instead of the raw Switch pointer.

The interface should also live in the peer package.

So, the Switch now has to comply with this peer-pkg interface.

Peer calls the interface's GetLink which returns ChannelLink, error

To implement this interface in unit tests, and have a non-nil ChannelLink** returned on GetLink, we must implement this interface in the peer.

ChannelLink interface has this method HandleSwitchPacket(*htlcPacket) error which uses a private *htlcPacket argument.

So there are two options:

Change the interface GetLink to not return ChannelLink, error

Change ChannelLink methods to not have *htlcPacket

So IMO this is a workaround until we can cleanup the htlcswitch package. One thing would be to remove *htlcPacket in public methods. And another would be to have GetLink return a concrete type so that it can fit into any interface because we don't have interfaces narrowing ChannelLink -> MessageLink. This last change would allow us to remove the channelSwitch type.

** I have a test-case that requires a non-nil link to be returned.

Is this a blocker for your ACK? I can figure out the best step to avoid this "type confusion"

If we want to be able to replace *htlcswitch.Switch in peer tests (which IMO is worth it, as it does not belong in the peer unit tests):

I agree we should do this. And what confused me here is that, the change made in current commits do not acheive this goal? You have a MessageSwitch interface defined, which is good. But the concrete implementation still requires a *htlcswitch.Switch. The test below uses NewChannelSwitch, which takes a *htlcswitch.Switch.

The interface should also live in the peer package.
So, the Switch now has to comply with this peer-pkg interface.

This is probably my main blocker because, imo, packages should be independent in the sense of actual implementations of interfaces. peer should not care about the actual implementation of an interface defined by switch, and vice versa. peer should use the interfaces like its an API, only cares about what arguments to use and what values are returned. This way we could provide a proper abstraction for each package.

Peer calls the interface's GetLink which returns ChannelLink, error
To implement this interface in unit tests, and have a non-nil ChannelLink** returned on GetLink, we must implement this interface in the peer.
ChannelLink interface has this method HandleSwitchPacket(*htlcPacket) error which uses a private *htlcPacket argument.

You could also make *htlcPacket an interface so we don't need the concrete type? Just like you've mentioned,

One thing would be to remove *htlcPacket in public methods.

The tests do not use NewChannelSwitch, they use the mockMessageSwitch? The peer interface will always be a subset of functionality provided by the Switch and only cares about the functions used in the peer package so I think this is achieved? I think why I didn't change *htlcPacket was because it was a pretty large change that touched the unit tests of the htlcswitch package

yyforyongyu · 2021-07-05T01:26:21Z

peer/brontide.go

+// ChannelSwitch is an implementation of the MessageSwitch interface that wraps
+// htlcswitch.Switch.
+type ChannelSwitch struct {
+	innerSwitch *htlcswitch.Switch


Still reading through the commits, but I think the innerSwitch and *htlcswitch.Switch are the same thing in terms of structure right?

Do we need the innerSwitch variable at all? We could just embed.

yyforyongyu · 2021-07-05T01:27:25Z

peer/brontide.go

+	return s.innerSwitch.GetLink(cid)
+}
+
+// InitLink initializes a ChannelLink in the Switch.


MessageLink or ChannelLink?

This is using the underlying htlcswitch method AddLink and NewChannelLink, so I'd say a ChannelLink

peer/test_utils.go

peer/brontide_test.go

yyforyongyu · 2021-07-05T01:55:29Z

peer/interfaces.go

@@ -88,3 +90,22 @@ type MessageSwitch interface {
 	// ChannelID.
 	RemoveLink(lnwire.ChannelID)
 }
+
+// ChannelGraph is an interface that abstracts the network graph.
+type ChannelGraph interface {


Maybe we could change the names to something else as a) the same names are confusing and b) the name here means an interface but in channeldb it's a concrete type, which is more confusing.

We do this in netann and autopilot for ChannelGraph interface. We should be using package interfaces in most places to reduce dependencies on other packages, so IMO similar naming across packages shouldn't matter.

bhandras

Nice changes that'll make testing much simper! 🥇 Added a few comments and suggestions but overall the PR is almost ready.

bhandras · 2021-07-05T13:57:57Z

peer/interfaces.go

+
+// MessageLink is an interface that contains some functionality from a
+// htlcswitch.ChannelLink.
+type MessageLink interface {


To be more idiomatic Go, maybe we could name this interface ChannelUpdateHandler. Since it's handling channel updates. wdyt?

Prefer to keep it as MessageLink

bhandras · 2021-07-05T13:59:10Z

peer/brontide.go

+
+// MakeChannelSwitch initializes a ChannelSwitch given a raw *htlcswitch.Switch
+// pointer.
+func MakeChannelSwitch(innerSwitch *htlcswitch.Switch) *ChannelSwitch {


bhandras · 2021-07-05T13:59:54Z

peer/brontide.go

+// ChannelSwitch is an implementation of the MessageSwitch interface that wraps
+// htlcswitch.Switch.
+type ChannelSwitch struct {
+	innerSwitch *htlcswitch.Switch


Do we need the innerSwitch variable at all? We could just embed.

peer/test_utils.go

bhandras · 2021-07-05T14:05:52Z

peer/brontide_test.go

+			// Check that mockConn sent out the expected message.
+			msg, err := getMessage(mockConn)
+			require.NoError(t, err)
+			require.Equal(t, msg, test.expectedInit)


nit: the recommended order is reqire.Equal(t, expected, got)

changed in the commit it's introduced, a later commit removes it

bhandras · 2021-07-05T14:07:09Z

watchtower/wtmock/peer.go

+func (p *MockPeer) ReadNextBody(_ []byte) ([]byte, error) {
+	select {
+	case msg := <-p.IncomingMsgs:
+		return msg, nil


nit: new line before the next case branch

bhandras · 2021-07-05T14:07:37Z

watchtower/wtmock/peer.go

+	select {
+	case msg := <-p.IncomingMsgs:
+		return msg, nil
+	case <-time.After(5 * time.Second):


nit: could make this a const

bhandras · 2021-07-05T14:08:27Z

lntest/mock/walletcontroller.go

+	if addrType == lnwallet.WitnessPubKey {
+		witProgram := btcutil.Hash160(key)
+
+		addr, _ := btcutil.NewAddressWitnessPubKeyHash(


why swallow the error if we might as well just return it?

returning error

bhandras · 2021-07-05T14:13:38Z

peer/brontide_test.go

+	// Alice should reply with an Init message, we discard it.
+	msg, err := getMessage(aliceConn)
+	require.NoError(t, err)
+	_, ok := msg.(*lnwire.Init)


nit: instead of these casts, we could use reqire.IsType to make the tests more readable.

Changed only for the Init messages, other message types are cast and then the fields are used

Crypt-iQ · 2021-07-06T15:50:29Z

@yyforyongyu Interfaces live in the peer package so testing the peer is easier, if there are global interfaces they could live in another package. But interface use across packages will just lead to import cycles and not very good separation of what a specific package needs (usually a subset of functionality)

Crypt-iQ · 2021-07-08T18:43:10Z

Another issue with having inter-package dependencies is that it makes it impossible to fuzz or unit test a specific package without including all of its dependencies. So this PR removes that friction by not having to instantiate a whole *htlcswitch.Switch when unit testing, among other out-of-package structs

Otherwise, the Peer will send Shutdown messages with incorrect DeliveryAddress.

This commit removes the references to internal Brontide members and instead creates *Brontide and only accesses public members. This allows for proper testing of the Brontide API and allows us to send messages as a normal peer would. This also paves the way for moving the peer tests to the peer_test package instead of residing in the peer package.

yyforyongyu

Still going through the last commit.

Interfaces live in the peer package so testing the peer is easier

Could you elaborate? As long as it's an interface, I don't think it matters because it's an abstraction, and you really shouldn't test abstractions in unit tests (that's integration test or functional test are for).

But interface use across packages will just lead to import cycles and not very good separation of what a specific package needs (usually a subset of functionality)

It depends. go's io package is a good example, it provides a reader/writer interface then the package bytes implements it in its buffer struct. In our case, interface living in htlcswitch means it provides the abstractions that can be used by other packages without caring about the implementation details. This is also why we want to use mock here, as we don't want to re-implement the logic in our unit tests.

Another issue with having inter-package dependencies is that it makes it impossible to fuzz or unit test a specific package without including all of its dependencies. So this PR removes that friction by not having to instantiate a whole *htlcswitch.Switch when unit testing, among other out-of-package structs

I think that's why we use mock.

yyforyongyu · 2021-07-13T09:37:04Z

peer/interfaces.go

+
+// MessageSwitch is an interface that manages setup, retrieval, and shutdown of
+// MessageLink implementations.
+type MessageSwitch interface {


I don't think you could get over *htlcswitch.Switch since channelSwitch needs it. This design pattern is weird. You have a concrete type defined in another package, and you have an interface defined in this package, then you create a concrete struct that implements the interface, which in the end is the concrete type living in another package.

peer/test_utils.go

yyforyongyu · 2021-07-13T09:48:57Z

peer/interfaces.go

+// Sphinx is an interface that abstracts the decryption of onion blobs.
+type Sphinx interface {
+	// DecodeHopIterators batch decodes HTLC onion blobs.
+	DecodeHopIterators([]byte, []hop.DecodeHopIteratorRequest) (


We usually don't add args to interfaces

I don't think that's the case, curious where did you get that?

Crypt-iQ · 2021-07-13T16:46:49Z

Still going through the last commit.

Interfaces live in the peer package so testing the peer is easier

Could you elaborate? As long as it's an interface, I don't think it matters because it's an abstraction, and you really shouldn't test abstractions in unit tests (that's integration test or functional test are for).

The abstractions are used so we don't have to make concrete objects that are irrelevant for testing the peer.

But interface use across packages will just lead to import cycles and not very good separation of what a specific package needs (usually a subset of functionality)

It depends. go's io package is a good example, it provides a reader/writer interface then the package bytes implements it in its buffer struct. In our case, interface living in htlcswitch means it provides the abstractions that can be used by other packages without caring about the implementation details. This is also why we want to use mock here, as we don't want to re-implement the logic in our unit tests.

If htlcswitch exports a Switch interface or ChannelLink interface that is then used by the peer package, then peer imports htlcswitch. If htlcswitch then imports peer package (or a package that uses peer indirectly) to make use of some interface living in the peer, then we have an import cycle/

Crypt-iQ · 2021-07-13T17:09:13Z

I think that's why we use mock.

Mock does not solve this. The peer requires concrete structs which should not be the case if we want to unit/fuzz test this. The way to solve this is with interfaces.

yyforyongyu

Written below my personal opinions, non-blocking,

The abstractions are used so we don't have to make concrete objects that are irrelevant for testing the peer.

Agreed. And I like this new approach of interfacing wherever possible. The issue is, who is reponsible for providing and maintaining the abstractions? Why is peer defining the abstractions while the acutal implementaions are living across various packages? This also means that peer is not pure in the sense that it needs to know all the implementation details of other packages?

If htlcswitch exports a Switch interface or ChannelLink interface that is then used by the peer package, then peer imports htlcswitch. If htlcswitch then imports peer package (or a package that uses peer indirectly) to make use of some interface living in the peer, then we have an import cycle/

If we have this issue we should fix it. Also I think we should forbid it rather than allowing it to happen. Plus I don't think it's a better solution this way. If the interface is exported by switch, then used by peer, then imported again by switch, the switch should not import from peer but use its own defined interfaces.

yyforyongyu · 2021-08-02T09:34:29Z

peer/brontide_test.go


-	respDeliveryScript := shutdownMsg.Address
+	// Create a test channel between Alice and Bob.
+	aliceChan, bobLnChan, err := createTestChannels(noUpdate,


We could also pass the ctx to function createTestChannels to avoid the long function signature.

yyforyongyu · 2021-08-02T09:39:19Z

peer/test_utils.go

+	alicePath, err := ioutil.TempDir("", "alicedb")
+	require.NoError(t, err)
+
+	aliceDb, err := channeldb.Open(alicePath)


Looks like we want to deprecate the Open function @bhandras ?

yyforyongyu · 2021-08-02T09:51:11Z

peer/test_utils.go

-		t:               t,
-		writtenMessages: make(chan []byte, expectedMessages),
+// pushMessage pushes an lnwire.Message to the MockPeer.
+func pushMessage(p *wtmock.MockPeer, msg lnwire.Message) error {


We could change this into assertPushMessage and use the require to check the error here, which has the benefit of reducing half of the lines.

yyforyongyu · 2021-08-02T09:52:18Z

peer/test_utils.go

-// slice we expect.
-func (m *mockMessageConn) assertWrite(expected []byte) {
+// getMessage retrieves a message from the MockPeer.
+func getMessage(p *wtmock.MockPeer) (lnwire.Message, error) {


Similarly, this could also be an assertion to reduce roughly two thrids of the lines.

Future tests may not know the message coming across, so I can change the pushMessage but not the getMessage

yyforyongyu · 2021-08-02T10:13:11Z

peer/brontide_test.go

-			if !bytes.Equal(test.expectedScript, shutdownMsg.Address) {
+			// Check that the Shutdown message includes the
+			// expected delivery script.
+			if !bytes.Equal(


Use require instead?

yyforyongyu · 2021-08-02T11:09:35Z

peer/interfaces.go

+
+// MessageSwitch is an interface that manages setup, retrieval, and shutdown of
+// MessageLink implementations.
+type MessageSwitch interface {


If we want to be able to replace *htlcswitch.Switch in peer tests (which IMO is worth it, as it does not belong in the peer unit tests):

I agree we should do this. And what confused me here is that, the change made in current commits do not acheive this goal? You have a MessageSwitch interface defined, which is good. But the concrete implementation still requires a *htlcswitch.Switch. The test below uses NewChannelSwitch, which takes a *htlcswitch.Switch.

The interface should also live in the peer package.
So, the Switch now has to comply with this peer-pkg interface.

This is probably my main blocker because, imo, packages should be independent in the sense of actual implementations of interfaces. peer should not care about the actual implementation of an interface defined by switch, and vice versa. peer should use the interfaces like its an API, only cares about what arguments to use and what values are returned. This way we could provide a proper abstraction for each package.

Peer calls the interface's GetLink which returns ChannelLink, error
To implement this interface in unit tests, and have a non-nil ChannelLink** returned on GetLink, we must implement this interface in the peer.
ChannelLink interface has this method HandleSwitchPacket(*htlcPacket) error which uses a private *htlcPacket argument.

You could also make *htlcPacket an interface so we don't need the concrete type? Just like you've mentioned,

One thing would be to remove *htlcPacket in public methods.

Crypt-iQ · 2021-08-02T14:52:07Z

This also means that peer is not pure in the sense that it needs to know all the implementation details of other packages?

So I think this is mainly where we disagree -- the peer only cares about the functions and return values it uses. It does not use every method of the *htlcswitch.Switch and therefore does not know implementation details of other packages. This would mean its pure like you said

Crypt-iQ · 2021-08-02T15:07:27Z

If the interface is exported by switch, then used by peer, then imported again by switch, the switch should not import from peer but use its own defined interfaces.

If the peer is importing an interface defined by the switch, and the switch imports a different interface from the peer (or an indirect package that also imports the peer package), we get this problem. This is the problem with importing from other packages that we want to solve. This is why each package needs its own interfaces that are not used by other packages.

yyforyongyu · 2021-08-03T00:00:39Z

If the peer is importing an interface defined by the switch, and the switch imports a different interface from the peer (or an indirect package that also imports the peer package), we get this problem. This is the problem with importing from other packages that we want to solve. This is why each package needs its own interfaces that are not used by other packages.

Like I said this shouldn't happen in the first place. And imo we should NEVER import like how you've described above. Having an import cycle is bad, allowing it to happen is worse. If we do have this issue then there's something wrong with the code structure, and we should fix that rather than enabling it to happen.

So I think this is mainly where we disagree -- the peer only cares about the functions and return values it uses. It does not use every method of the *htlcswitch.Switch and therefore does not know implementation details of other packages. This would mean its pure like you said

The peer not only holds interfaces like MessageLink and MessageSwitch, it also implements them inside the peer package, how's that counted as not knowing implementation details of other packages? And for the switch package, whenever we make changes to the functions, we now need to look at the peer package to decide whether we are satisfying the interfaces or not, doesn't this make both the peer and the switch less pure?

I think the effective go has made it clear, and it has a nice example of how crypto/cipher uses Block interface in the end,

If a type exists only to implement an interface and will never have exported methods beyond that interface, there is no need to export the type itself. Exporting just the interface makes it clear the value has no interesting behavior beyond what is described in the interface. It also avoids the need to repeat the documentation on every instance of a common method.

Crypt-iQ · 2021-08-03T15:11:50Z

The peer not only holds interfaces like MessageLink and MessageSwitch, it also implements them inside the peer package, how's that counted as not knowing implementation details of other packages? And for the switch package, whenever we make changes to the functions, we now need to look at the peer package to decide whether we are satisfying the interfaces or not, doesn't this make both the peer and the switch less pure?

The peer package only implements what it uses, it does not implement all methods of the switch -- if the peer already used a function A from the switch package, changing the method would require a change in the peer. Likewise if the peer uses a peer-specific interface which contains the function A, and the switch no longer complies with the interface, a change in the peer is also necessary.

So I'll just give some examples to show my point:

contractcourt having an interface for Registry which is implemented by the invoice registry in the invoices package and also used in contractcourt/mock_registry_test.go:

lnd/contractcourt/interfaces.go

Lines 18 to 35 in a329c80

    
           type Registry interface { 
        
           	// LookupInvoice attempts to look up an invoice according to its 32 
        
           	// byte payment hash. 
        
           	LookupInvoice(lntypes.Hash) (channeldb.Invoice, error) 
        
           	// NotifyExitHopHtlc attempts to mark an invoice as settled. If the 
        
           	// invoice is a debug invoice, then this method is a noop as debug 
        
           	// invoices are never fully settled. The return value describes how the 
        
           	// htlc should be resolved. If the htlc cannot be resolved immediately, 
        
           	// the resolution is sent on the passed in hodlChan later. 
        
           	NotifyExitHopHtlc(payHash lntypes.Hash, paidAmount lnwire.MilliSatoshi, 
        
           		expiry uint32, currentHeight int32, 
        
           		circuitKey channeldb.CircuitKey, hodlChan chan<- interface{}, 
        
           		payload invoices.Payload) (invoices.HtlcResolution, error) 
        
           	// HodlUnsubscribeAll unsubscribes from all htlc resolutions. 
        
           	HodlUnsubscribeAll(subscriber chan<- interface{}) 
        
           }

htlcswitch having an interface InvoiceDatabase which is implemented by the invoice registry in the invoices package and is also used in htlcswitch/mock.go:

lnd/htlcswitch/interfaces.go

Lines 16 to 42 in 47732ec

    
           type InvoiceDatabase interface { 
        
           	// LookupInvoice attempts to look up an invoice according to its 32 
        
           	// byte payment hash. 
        
           	LookupInvoice(lntypes.Hash) (channeldb.Invoice, error) 
        
           	// NotifyExitHopHtlc attempts to mark an invoice as settled. If the 
        
           	// invoice is a debug invoice, then this method is a noop as debug 
        
           	// invoices are never fully settled. The return value describes how the 
        
           	// htlc should be resolved. If the htlc cannot be resolved immediately, 
        
           	// the resolution is sent on the passed in hodlChan later. The eob 
        
           	// field passes the entire onion hop payload into the invoice registry 
        
           	// for decoding purposes. 
        
           	NotifyExitHopHtlc(payHash lntypes.Hash, paidAmount lnwire.MilliSatoshi, 
        
           		expiry uint32, currentHeight int32, 
        
           		circuitKey channeldb.CircuitKey, hodlChan chan<- interface{}, 
        
           		payload invoices.Payload) (invoices.HtlcResolution, error) 
        
           	// CancelInvoice attempts to cancel the invoice corresponding to the 
        
           	// passed payment hash. 
        
           	CancelInvoice(payHash lntypes.Hash) error 
        
           	// SettleHodlInvoice settles a hold invoice. 
        
           	SettleHodlInvoice(preimage lntypes.Preimage) error 
        
           	// HodlUnsubscribeAll unsubscribes from all htlc resolutions. 
        
           	HodlUnsubscribeAll(subscriber chan<- interface{}) 
        
           }

lnwallet having an interface BlockChainIO that is implemented by a chain-backend. This one isn't used in tests (but could be via mocking). Some of these interfaces may have been originally used to avoid import cycles.

lnd/lnwallet/interface.go

Lines 412 to 435 in 44971f0

    
           type BlockChainIO interface { 
        
           	// GetBestBlock returns the current height and block hash of the valid 
        
           	// most-work chain the implementation is aware of. 
        
           	GetBestBlock() (*chainhash.Hash, int32, error) 
        
           	// GetUtxo attempts to return the passed outpoint if it's still a 
        
           	// member of the utxo set. The passed height hint should be the "birth 
        
           	// height" of the passed outpoint. The script passed should be the 
        
           	// script that the outpoint creates. In the case that the output is in 
        
           	// the UTXO set, then the output corresponding to that output is 
        
           	// returned.  Otherwise, a non-nil error will be returned. 
        
           	// As for some backends this call can initiate a rescan, the passed 
        
           	// cancel channel can be closed to abort the call. 
        
           	GetUtxo(op *wire.OutPoint, pkScript []byte, heightHint uint32, 
        
           		cancel <-chan struct{}) (*wire.TxOut, error) 
        
           	// GetBlockHash returns the hash of the block in the best blockchain 
        
           	// at the given height. 
        
           	GetBlockHash(blockHeight int64) (*chainhash.Hash, error) 
        
           	// GetBlock returns the block in the main chain identified by the given 
        
           	// hash. 
        
           	GetBlock(blockHash *chainhash.Hash) (*wire.MsgBlock, error) 
        
           }

This pull:
- defines interfaces that live in the peer package and only consist of the methods that the peer needs.
- tests those interfaces with mocks

Like I said this shouldn't happen in the first place. And imo we should NEVER import like how you've described above. Having an import cycle is bad, allowing it to happen is worse. If we do have this issue then there's something wrong with the code structure, and we should fix that rather than enabling it to happen.

Anecdotally, our code structure in lnd allows this to happen because we are importing interfaces across packages instead of having either:

global interfaces (which we want to work toward w/o having import cycles)
package-specific interfaces consisting of only what methods the package needs

Crypt-iQ · 2021-08-03T15:53:20Z

Going to open another pull that refactors the peer/switch interaction specifically and then base this PR on that one. Should clean some stuff up.

yyforyongyu · 2021-08-04T11:26:40Z

So I'll just give some examples to show my point:

Cool this is good to know. As mentioned above this is non-blocking and it's only personal opinions. I don't think there's a hard rule for deciding where the interfaces should live, only recommendations from practices like the effect go. THB it's really case-specific.

Anecdotally, our code structure in lnd allows this to happen because we are importing interfaces across packages

Unrelated to this PR, after reading the codebase these days I'm starting to have a vague idea of how the different layers are defined and interacted. Still learning, but will try to draft out the overall structure someday.

Crypt-iQ · 2021-08-04T14:21:08Z

So I'll just give some examples to show my point:

Cool this is good to know. As mentioned above this is non-blocking and it's only personal opinions. I don't think there's a hard rule for deciding where the interfaces should live, only recommendations from practices like the effect go. THB it's really case-specific.

It's good to have discussions about this as it forces me to defend my position and if I'm wrong, that's good as then I can re-evaluate my position. One goal I have for the codebase is to put global mocks in the lntest/mock package, and also to have global interfaces where possible, structured to avoid import cycles. This should make unit and fuzz testing a lot easier without needing to make structs like *htlcswitch.Switch or InvoiceRegistry (which is done in some unit tests).

yyforyongyu · 2022-01-07T02:37:28Z

Is this ready for another round?

Crypt-iQ · 2022-01-07T15:24:46Z

No this is pretty low prio for me atm

Crypt-iQ · 2022-01-14T15:47:16Z

!lightninglabs-deploy mute 2022-Feb-01

lightninglabs-deploy · 2022-02-08T01:34:22Z

@cfromknecht: review reminder
@Crypt-iQ, remember to re-request review from reviewers when ready

Crypt-iQ · 2022-02-08T15:21:50Z

!lightninglabs-deploy mute 2022-March-01

Crypt-iQ added p2p Code related to the peer-to-peer behaviour testing Improvements/modifications to the test suite interface abstraction refactoring labels Mar 3, 2021

Crypt-iQ added this to the 0.13.0 milestone Mar 3, 2021

Crypt-iQ added the v0.13 label Mar 4, 2021

cfromknecht reviewed Mar 10, 2021

View reviewed changes

peer/test_utils.go Outdated Show resolved Hide resolved

peer/test_utils.go Show resolved Hide resolved

peer/test_utils.go Show resolved Hide resolved

Roasbeef added the P3 might get fixed, nice to have label Mar 31, 2021

Crypt-iQ force-pushed the peerwrapperswitch_01152021 branch from d0924dc to c9f2a36 Compare April 1, 2021 21:24

Crypt-iQ requested a review from cfromknecht April 1, 2021 21:26

Roasbeef modified the milestones: 0.13.0, v0.14.0 Apr 6, 2021

Crypt-iQ force-pushed the peerwrapperswitch_01152021 branch from c9f2a36 to ef842ec Compare April 8, 2021 22:03

Crypt-iQ force-pushed the peerwrapperswitch_01152021 branch from ef842ec to ebb2ad5 Compare April 22, 2021 15:15

Roasbeef requested review from bhandras and yyforyongyu July 1, 2021 01:45

yyforyongyu reviewed Jul 5, 2021

View reviewed changes

bhandras reviewed Jul 5, 2021

View reviewed changes

Crypt-iQ removed the v0.13 label Jul 8, 2021

Crypt-iQ force-pushed the peerwrapperswitch_01152021 branch from ebb2ad5 to e8ab671 Compare July 8, 2021 19:55

Crypt-iQ added 2 commits July 8, 2021 16:05

htlcswitch+peer: pass in BestHeight to ChannelLinkConfig

49e417e

peer: introduce and use MessageLink interface in place of ChannelLink

e5bf234

Crypt-iQ added 2 commits July 8, 2021 16:05

lntest/mock: extend mockWalletController with P2WPKH

17f253b

Otherwise, the Peer will send Shutdown messages with incorrect DeliveryAddress.

Crypt-iQ force-pushed the peerwrapperswitch_01152021 branch from e8ab671 to 8390f7f Compare July 8, 2021 20:05

yyforyongyu reviewed Jul 13, 2021

View reviewed changes

yyforyongyu reviewed Aug 2, 2021

View reviewed changes

Crypt-iQ mentioned this pull request Aug 3, 2021

htlcswitch+peer: introduce packetHandler, ChannelUpdateHandler interfaces for cleaner separation #5603

Merged

Roasbeef modified the milestones: v0.14.0, v0.15.0 Aug 30, 2021

Crypt-iQ mentioned this pull request Nov 10, 2021

mock: Add mockgen workflow lightninglabs/pool#313

Merged

Crypt-iQ removed this from the v0.15.0 milestone Mar 25, 2022

Crypt-iQ closed this Jul 27, 2023

peer: more interfaces, refactor unit tests #5067

peer: more interfaces, refactor unit tests #5067

Conversation

Crypt-iQ commented Mar 3, 2021

cfromknecht left a comment

Choose a reason for hiding this comment

cfromknecht commented Apr 7, 2021

Crypt-iQ commented Apr 9, 2021 • edited Loading

yyforyongyu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Crypt-iQ Jul 13, 2021 • edited Loading

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

Crypt-iQ Jul 8, 2021 • edited Loading

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

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

Crypt-iQ commented Jul 6, 2021

Crypt-iQ commented Jul 8, 2021 • edited Loading

yyforyongyu left a comment

Choose a reason for hiding this comment

yyforyongyu Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ commented Jul 13, 2021

Crypt-iQ commented Jul 13, 2021

yyforyongyu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ commented Aug 2, 2021

Crypt-iQ commented Aug 2, 2021

yyforyongyu commented Aug 3, 2021

Crypt-iQ commented Aug 3, 2021

Crypt-iQ commented Aug 3, 2021

yyforyongyu commented Aug 4, 2021

Crypt-iQ commented Aug 4, 2021

yyforyongyu commented Jan 7, 2022

Crypt-iQ commented Jan 7, 2022

Crypt-iQ commented Jan 14, 2022

lightninglabs-deploy commented Feb 8, 2022

Crypt-iQ commented Feb 8, 2022

Crypt-iQ commented Apr 9, 2021 •

edited

Loading

yyforyongyu Jul 13, 2021 •

edited

Loading

Crypt-iQ Jul 13, 2021 •

edited

Loading

Crypt-iQ Jul 8, 2021 •

edited

Loading

Crypt-iQ commented Jul 8, 2021 •

edited

Loading

yyforyongyu Jul 13, 2021 •

edited

Loading