assets: harden tapd client lifecycle and RFQ handling - #1189
Conversation
25dd676 to
d8910ea
Compare
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
LGTM! 💾
Left come comments. The main one is hardening for GetRfqForAsset.
| if d.assetClient != nil { | ||
| d.assetClient.Close() | ||
| d.assetClient = nil | ||
| } |
There was a problem hiding this comment.
I propose to add defer inside if d.cfg.Tapd.Activate block above and close it there.
| // seconds accepted by tapd. Sub-second precision is rounded up so a positive | ||
| // timeout can never become tapd's invalid zero value. |
There was a problem hiding this comment.
Isn't there a stronger reason to do it so we don't get a timeout error?
If we account on 1.5s timeout (and actively using it, e.g. use it at time 1.4s, i.e. 0.1s before the timeout), but tapd enforces the timeout of just 1s (rounded down, as before), we get a timeout error. So we round up to request enough time, right?
| } | ||
| if coefficient.Sign() <= 0 { | ||
| return 0, fmt.Errorf("asset rate coefficient must be positive") | ||
| } |
There was a problem hiding this comment.
We need a similar hardening for GetRfqForAsset. Here is an example how to fix it: https://gist.github.com/starius/c23dd24330c3d36c9e852d2f4596313f
There was a problem hiding this comment.
extracted the checks and re-used it for GetFrqForAsset.
|
|
||
| c.assetNameMutex.Lock() | ||
| defer c.assetNameMutex.Unlock() | ||
| assetIdStr := hex.EncodeToString(assetId) | ||
| c.assetNameMutex.RLock() | ||
| if name, ok := c.assetNameCache[assetIdStr]; ok { | ||
| c.assetNameMutex.RUnlock() | ||
| return name, nil | ||
| } | ||
| c.assetNameMutex.RUnlock() | ||
|
|
There was a problem hiding this comment.
Let's preserve the defer pattern somehow. Maybe make private accessor methods for this map? To encapsulate map+mutex work there.
Close the TapdClient when daemon initialization fails, during normal shutdown, and after the view command completes. This prevents gRPC transport resources from leaking across embedded daemon lifecycles and error paths.
Convert the configured duration once during client creation. Round positive fractional durations up to the whole seconds accepted by tapd. Reject zero, negative, and overflowing values, and cover the conversion boundaries with unit tests.
Restrict the asset-name cache mutex to map access so a slow QueryAssetStats call cannot block cached readers. Use an RWMutex for independent cache reads and add a concurrent regression test.
Validate the rate pointer and decimal coefficient before converting asset units. Return errors for nil, malformed, non-positive, and oversized-scale rates instead of allowing nil dereferences or division-by-zero panics. Add regression tests for each case.
Validate the bid rate before returning an accepted asset sell quote. This prevents malformed rates from reaching downstream quote arithmetic, where nil or non-positive values can panic. Cover valid and malformed responses with table-driven tests.
Document the RFQ validation, cache responsiveness, and tapd connection lifecycle fixes in the rolling release notes.
d8910ea to
b686d97
Compare
|
/gateway review |
|
👀 gateway review starting… |
2 participants
Summary
Improve the tapd client’s lifecycle, timeout handling, concurrency, and validation.
viewcompletion.