StaticAddr: fix deposit detection #864
Conversation
hieblmi
force-pushed
the
fix-deposit-detection
branch
from
b5cba1f to
d8953be
Compare
hieblmi
force-pushed
the
fix-deposit-detection
branch
from
d8953be to
7b6e602
Compare
staticaddr/address/interface.go
Outdated
| // into the store. | ||
| CreateStaticAddress(ctx context.Context, addrParams *Parameters) error | ||
| CreateStaticAddress(ctx context.Context, addrParams *Parameters, | ||
| currentHeight int32) error |
There was a problem hiding this comment.
Why a separate argument currentHeight is needed? addrParams has new field InitiationHeight.
There was a problem hiding this comment.
removed currentHeight and use the InitiationHeight.
hieblmi
force-pushed
the
fix-deposit-detection
branch
from
7b6e602 to
23cc7ca
Compare
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
LGTM! 🏆
I propose to add a test reproducing the bug being fixed.
| ProtocolVersion: version.AddressProtocolVersion( | ||
| protocolVersion, | ||
| ), | ||
| InitiationHeight: m.currentHeight, |
There was a problem hiding this comment.
Can currentHeight be uninitialized (0) at this point? IMHO we should either return an error here if currentHeight is 0 (if we are sure that this won't happen) or wait for its initialization (e.g. using a cond var).
There was a problem hiding this comment.
For now I've added a check on entry of NewAddress.
staticaddr/address/manager_test.go
Outdated
| go func() { | ||
| err := testContext.manager.Run(ctxb) | ||
| require.NoError(t, err) | ||
| require.ErrorContains(t, err, "context canceled") |
There was a problem hiding this comment.
nit: require.ErrorIs(t, err, context.Canceled)
hieblmi
force-pushed
the
fix-deposit-detection
branch
from
23cc7ca to
b1c19e8
Compare
3 participants
The height hint for deposit confirmations was previously set to the initiation height of the deposit manager, which does not work in case the manager is restarted before a deposit was confirmed.