Conversation
There was a problem hiding this comment.
Pull request overview
This PR centralizes “retryable error” handling and reduces direct HTTP/JSON parsing by routing more network-asset reads through common.SafeReadAssetUntilSufficient, while also standardizing retry loops to use mtg.CheckRetryableError.
Changes:
- Replace direct Mixin Network asset HTTP calls with
common.SafeReadAssetUntilSufficientin observer/keeper/cmd paths. - Standardize transient error retry logic across Mixin/Ethereum/Bitcoin RPC call loops via
mtg.CheckRetryableError. - Add an Ethereum
FilterLogsretry helper (filterLogsUntilSufficient) and use it for ERC20 transfer log retrieval.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| observer/bond.go | Switch asset fetch + retry gating to shared helpers (SafeReadAssetUntilSufficient, mtg.CheckRetryableError). |
| mtg/utils.go | Expand/normalize retryable error string matching using lowercase comparisons. |
| keeper/network.go | Switch Mixin asset fetch + retry gating to shared helpers. |
| common/wallet.go | Treat retryable UTXO listing failures as non-fatal and continue loop. |
| cmd/transaction.go | Use SafeReadAssetUntilSufficient for CLI asset-id lookup; add context. |
| apps/mixin/rpc.go | Replace local retry string matching with mtg.CheckRetryableError. |
| apps/ethereum/rpc.go | Use shared retry logic and add filterLogsUntilSufficient for log filtering. |
| apps/bitcoin/rpc.go | Replace local retry string matching with mtg.CheckRetryableError. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| asset, err := common.SafeReadAssetUntilSufficient(ctx, mixinId) | ||
| if err != nil { | ||
| panic(mixinId) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| var body struct { | ||
| Data struct { | ||
| AssetId string `json:"asset_id"` | ||
| MixinId string `json:"mixin_id"` | ||
| } `json:"data"` | ||
| } | ||
| json.NewDecoder(resp.Body).Decode(&body) | ||
| if body.Data.MixinId != mixinId { | ||
| if asset.KernelAssetID != mixinId { | ||
| panic(mixinId) | ||
| } | ||
| return body.Data.AssetId | ||
| return asset.AssetID |
There was a problem hiding this comment.
SafeReadAssetUntilSufficient returns (nil, nil) on 404 (see common/mixin.go), so asset can be nil here; dereferencing asset.KernelAssetID / asset.AssetID will panic. Handle the asset == nil case explicitly (e.g., panic with a clearer message or return an empty string / error depending on expected CLI behavior).
| @@ -278,14 +254,10 @@ func (node *Node) fetchAssetMeta(ctx context.Context, id string) (*store.Asset, | |||
| if err == nil { | |||
There was a problem hiding this comment.
fetchMixinAsset can return (nil, nil) when the asset is not found (404), which makes meta nil here; calling WriteAssetMeta(ctx, meta) will panic because it dereferences asset. Add a meta == nil branch (similar to observer/bond.go) to return (nil, nil) without writing.
| if err == nil { | |
| if err == nil { | |
| if meta == nil { | |
| return nil, nil | |
| } |
| func filterLogsUntilSufficient(ctx context.Context, rpc string, query ethereum.FilterQuery) ([]types.Log, error) { | ||
| client, err := ethclient.Dial(rpc) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This dials an ethclient but never closes it; over time this can leak TCP connections/file descriptors (especially if called frequently). Ensure the client is closed (or reuse a shared client) and also consider breaking out when ctx is done to avoid retrying forever on context-related failures.
| for { | ||
| logs, err := client.FilterLogs(ctx, query) | ||
| if mtg.CheckRetryableError(err) { | ||
| time.Sleep(7 * time.Second) | ||
| continue | ||
| } | ||
| time.Sleep(7 * time.Second) | ||
| return logs, err |
There was a problem hiding this comment.
If client.FilterLogs returns a retryable error caused by the passed-in ctx (e.g., deadline exceeded), this loop will retry indefinitely with the same expired context. Add an explicit if ctx.Err() != nil { return nil, ctx.Err() } (or avoid treating context deadline errors as retryable) before sleeping/retrying.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3a8d672c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| logs, err := filterLogsUntilSufficient(ctx, rpc, ethereum.FilterQuery{ | ||
| FromBlock: big.NewInt(height), | ||
| ToBlock: big.NewInt(height), | ||
| }) |
There was a problem hiding this comment.
GetERC20TransferLogFromBlock now calls filterLogsUntilSufficient but never checks the returned err, so any non-retryable FilterLogs failure is silently treated as success and the function returns an empty transfer list. This can hide RPC/query failures (for example invalid filter params or provider-side errors) and cause missed ERC-20 transfer processing instead of surfacing an error to callers.
Useful? React with 👍 / 👎.
| err = json.NewDecoder(resp.Body).Decode(&body) | ||
| if err != nil { | ||
| func (node *Node) fetchMixinAsset(ctx context.Context, id string) (*store.Asset, error) { | ||
| asset, err := common.SafeReadAssetUntilSufficient(ctx, id) |
There was a problem hiding this comment.
This change removes use of node.conf.MixinMessengerAPI from keeper asset lookups by switching to common.SafeReadAssetUntilSufficient(ctx, id), which takes no API endpoint argument. As a result, deployments that rely on a non-default mixin-messenger-api value will no longer query their configured endpoint here, and asset metadata fetches can fail in those environments.
Useful? React with 👍 / 👎.
No description provided.