tapfreighter: fix OutboundParcel.Copy and add generic Copy fn test
#1580
Conversation
bhandras
force-pushed
the
copy-fixes
branch
3 times, most recently
from
3b02cbd to
97c5026
Compare
copy_test.go
Outdated
| ) | ||
|
|
||
| // FillFakeData recursively fills a struct with dummy values. | ||
| func FillFakeData[T any](t *testing.T, debug bool, maxDepth int, v T) { |
There was a problem hiding this comment.
nit: should we move these helper functions to the internal/test package? Then we can more easily re-use them in other places. And the copy test could actually be in the tapfreighter test.
copy_test.go
Outdated
| // CopyFnTest checks that the Copy method returns a value that: | ||
| // 1) is deeply equal | ||
| // 2) does not alias mutable fields (pointers, slices, maps) | ||
| func CopyFnTest[T fn.Copyable[T]](t *testing.T, debug, strict bool, |
There was a problem hiding this comment.
nit: rename to AssertCopyEqual() or something like that? Was initially not very clear to me what this function really does, at least from its name.
copy_test.go
Outdated
| } | ||
|
|
||
| // fillFakeData is the recursive helper to fill a value with fake data. | ||
| func fillFakeData(t *testing.T, debug bool, depth, maxDepth int, |
There was a problem hiding this comment.
Very nice! I wish the fuzz library exposed some of their helpers that are used to generate such random values. But nice to have such a compact version we can use here.
There was a problem hiding this comment.
Agreed, ideally something similar should be in the standard library.
| Inputs: fn.CopySlice(o.Inputs), | ||
| Outputs: fn.CopySlice(o.Outputs), | ||
| Label: o.Label, | ||
| SkipAnchorTxBroadcast: o.SkipAnchorTxBroadcast, |
There was a problem hiding this comment.
The initial bug that i found was that subscribing to send events did not correctly fill the confirmation height upon finished transfer.
tapfreighter/interface.go
Outdated
| TaprootAssetRoot: fn.CopySlice( | ||
| oldAnchor.TaprootAssetRoot, | ||
| ), | ||
| CommitmentVersion: &commitmentVersion, |
There was a problem hiding this comment.
This should remain nil (instead of becoming a pointer to a 0 value) if the original is nil.
The main goal of this commit is to give us a tool to be able to catch regressions when extending a copyable struct. We also fix
OutboundParcel.Copy()along the way.