-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(SPV-000): simplification of chainstate and proposed fixes for transaction #677
Conversation
Manual Testsℹ️ Remember to ask team members to perform manual tests and to assign |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #677 +/- ##
==========================================
- Coverage 49.01% 48.98% -0.03%
==========================================
Files 264 263 -1
Lines 12226 12152 -74
==========================================
- Hits 5992 5953 -39
+ Misses 5640 5605 -35
Partials 594 594
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
"github.com/bitcoin-sv/go-broadcast-client/broadcast" | ||
) | ||
|
||
func (chainstate *Client) submitTransaction(ctx context.Context, txID, txHex string, format HexFormatFlag) *BroadcastFailure { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the code moved from broadcast_providers.go
because I found that we have only one supported way so no "providers" abstraction is needed.
ctxWithCancel, cancel := context.WithTimeout(ctx, timeout) | ||
defer cancel() | ||
|
||
failure := c.submitTransaction(ctxWithCancel, txID, txHex, format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In previous implementation we was waiting for the first successful response from one of the providers. That's why there was additional goroutines and channels.
But:
- There was only one "provider" - the
broadcastClient
- So I'm proposing to simplyfy this logic without unnecessary goroutines and no "providers" abstraction.
// check in Mempool as fallback - if transaction is there -> GREAT SUCCESS | ||
if _, err := c.QueryTransaction(ctx, txID, requiredInMempool, timeout); err != nil { | ||
return &BroadcastFailure{ | ||
InvalidTx: failure.InvalidTx, | ||
Error: fmt.Errorf("query tx failed: %w, initial error: %s", err, failure.Error.Error()), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I don't understand this block of code 🙈
@@ -54,7 +54,7 @@ type ( | |||
// | |||
// If no options are given, it will use the defaultClientOptions() | |||
// ctx may contain a NewRelic txn (or one will be created) | |||
func NewClient(ctx context.Context, opts ...ClientOps) (ClientInterface, error) { | |||
func NewClient(ctx context.Context, logger *zerolog.Logger, opts ...ClientOps) (ClientInterface, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the logger as normal parameter instead of an option because in our implementation we expect the logger to exists here.
// Debug will set the debug flag | ||
func (c *Client) Debug(on bool) { | ||
c.options.debug = on | ||
} | ||
|
||
// DebugLog will display verbose logs | ||
func (c *Client) DebugLog(text string) { | ||
c.options.logger.Debug().Msg(text) | ||
} | ||
|
||
// IsDebug will return if debugging is enabled | ||
func (c *Client) IsDebug() bool { | ||
return c.options.debug | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed those methods, because - as far as I remember - we wanted to get rid of this approach, in favor of zerolog.Logger's
levels.
@@ -17,7 +18,7 @@ type HTTPInterface interface { | |||
// ChainService is the chain related methods | |||
type ChainService interface { | |||
SupportedBroadcastFormats() HexFormatFlag | |||
Broadcast(ctx context.Context, id, txHex string, format HexFormatFlag, timeout time.Duration) *BroadcastResult | |||
Broadcast(ctx context.Context, id, txHex string, format HexFormatFlag, timeout time.Duration) *BroadcastFailure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BroadcastResult
contained the BroadcastFailure
and the name of "provider".
- So, because of I removed the "providers" abstraction - the name became unused.
conditions := map[string]interface{}{ | ||
xPubIDField: m.XpubID, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition with xPubIDField
is not needed (redundant), because the conditions
map is passed to GetPaymailAddressesByXPubID
as "additional condition".
- In the
GetPaymailAddressesByXPubID
the field is set toxPubIDField: m.XpubID,
anyway.
@@ -111,27 +111,27 @@ func broadcastSyncTransaction(ctx context.Context, syncTx *SyncTransaction) erro | |||
client := syncTx.Client() | |||
chainstateSrv := client.Chainstate() | |||
|
|||
// Get the transaction HEX | |||
// Get the transaction from syncTx otherwise load it by ID from the database |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was misleading (to me) - so I've adjusted it.
tx := syncTx.transaction | ||
if tx == nil || tx.Hex == "" { | ||
if tx, err = _getTransaction(ctx, syncTx.ID, syncTx.GetOptions(false)); err != nil { | ||
return nil | ||
return nil // TODO: should we return an error here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we skip the error here?
- Maybe it's ok, but some comment would be great.
Duting analyss of transaction flow I found several places which can be simplified of fixed.
See my comments to the most important places of my changes.
Pull Request Checklist