feat: add Cosmos, Maya, and Tron chain support for tx_indexer - #473
feat: add Cosmos, Maya, and Tron chain support for tx_indexer#473jpthor wants to merge 6 commits into
Conversation
|
Warning Rate limit exceeded@jpthor has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 17 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| rpcs[common.Zcash] = zcashRpc | ||
| } | ||
|
|
||
| if cfg.Cosmos.URL != "" { |
There was a problem hiding this comment.
Please add new config values in deploy/dev/01_rpc.yaml and deploy/01_tx_indexer.yaml (Dev env). For a Prod env ask @RaghavSood to update config with new rpcs
gomesalexandre
left a comment
gomesalexandre
left a comment
There was a problem hiding this comment.
CI red — test job fails (old run from Feb). Build passing.
Rebase needed to pick up current main + fix test failures before this can merge.
Not approving with red CI.
|
@jpthor this PR has had CI failures for a while. Still relevant? If yes, can you rebase against main and address the test failures - the workflow is green but is failing. |
gomesalexandre
left a comment
gomesalexandre
left a comment
There was a problem hiding this comment.
Cosmos, Maya, and Tron tx_indexer support - good to see. the RPC implementations are straightforward but there are a couple of fund-safety issues with the pubkey extraction heuristic.
pubkey prefix heuristic is fragile and trust-breaking
preferably-blocking: extractCosmosPubKeyFromTx (and the Tron variant) detects the public key by checking if txBytes[0] == 0x02 || txBytes[0] == 0x03. this is a heuristic, not a protocol guarantee. any Cosmos protobuf-encoded transaction whose first byte happens to be 0x02 or 0x03 will have its first 33 bytes silently misinterpreted as a pubkey. protobuf field tags are 1-byte varint encoded and field numbers 1-3 with wire type 2 (length-delimited) produce bytes 0x0a, 0x12, 0x1a - so the first byte of a real tx.Tx is almost never 0x02/0x03, but "almost never" is not "never". a Cosmos MsgSend with a specific amount encoding could start with 0x02.
the correct approach is to use a documented envelope (e.g., a fixed 4-byte magic header + 33-byte pubkey prefix, or extract the pubkey from the SignerInfo in the protobuf). this is security-critical because a wrong pubkey leads to a wrong signed tx, and the tx_indexer would compute a txHash that doesn't match what was actually broadcast.
q: how does the existing Solana/BTC indexer pass the pubkey? if there's an established convention in this codebase (e.g., a metadata wrapper around proposedTx), Cosmos/Tron should follow it. the comment says "we expect the caller to embed the pubkey in a metadata envelope" but there's no validation of that envelope - just a byte[0] check.
NewCosmos/NewMaya/NewTron return error but can never error
suggestion: all three constructors return (*T, error) but the body always returns nil for the error. this forces callers to handle an error that never happens. either drop the error return (make it func NewCosmos(url string) *Cosmos) or add actual validation (URL non-empty, basic URL parse check). as-is it's misleading.
Cosmos 404 handling may mask actual server errors
should-fix: the Cosmos RPC client returns TxOnChainPending for any non-200, non-404 response only if the body can't be parsed. but the if resp.StatusCode != http.StatusOK branch returns an error for non-200 statuses OTHER than 404 (e.g., 500, 503). this is fine for a first pass. however for Maya, the code has the same behavior - 429 (rate limit) from the public node returns an error to the caller, which the tx_indexer presumably logs and retries. consider whether 429 should be treated as pending vs error. not a hard blocker, just flag it.
Tron public endpoint in dev config
suggestion: deploy/dev/01_rpc.yaml adds tron: "https://tron-rpc.publicnode.com" - a free public endpoint with rate limits. this is fine for dev but should be called out in the config comments so ops doesn't copy this into staging/prod unchanged.
no tests for ComputeTxHash
should-fix: there are no tests for CosmosIndexer.ComputeTxHash, MayaIndexer.ComputeTxHash, or TronIndexer.ComputeTxHash. these functions compute the txHash that gets stored and used for payout/settlement matching. a wrong hash is a fund-safety issue - if the stored hash doesn't match the broadcast txHash, payouts fail or go to wrong transactions. at minimum one happy-path test with a known signed tx + expected hash.
praise: the three-way status (pending/success/fail) mapping for Cosmos LCD API and Tron's blockNumber=0=pending heuristic are both correct. the Tron Result == "FAILED" + Receipt.Result == "FAILED" two-layer check covers both contract-level and network-level failures cleanly.
|
round 2 review - reading the branch directly (not just the diff), three new blockers surfaced on top of prior findings prior issues status
blocker: tron ComputeTxHash strips wrong number of bytes for uncompressed pubkeys
if len(proposedTx) > 33 && (proposedTx[0] == 0x02 || proposedTx[0] == 0x03) {
rawData = proposedTx[33:]
}
// 0x04 case: rawData is NOT strippedif an uncompressed pubkey is prefixed, fix: use the length returned by blocker: on main: UPDATE tx_indexer SET lost=$1, status_onchain=$2::..., error_message=$3, updated_at=now() WHERE id=$4on branch: UPDATE tx_indexer SET lost=$1, updated_at=now() WHERE id=$2timed-out txs will have blocker: main had chain-specific timeouts (solana=2m, xrp=5m, evm=30m). branch deletes the function and uses a single global. if the global config is shorter than 30m, mempool-pending EVM txs during congestion get marked lost prematurely. new Cosmos/Maya/Tron chains are fast-finality so the global probably works for them - but EVM regresses. preferably-blocking: cosmosSDK := cosmos.NewSDK(nil)
chains[common.GaiaChain] = chain.NewCosmosIndexer(cosmosSDK)
chains[common.MayaChain] = chain.NewMayaIndexer(cosmosSDK)if preferably-blocking:
pubKey, _ := extractCosmosPubKeyFromTx(proposedTx) // extracts first 33 bytes
signed, _ := c.sdk.Sign(proposedTx, sigs, pubKey) // passes full bytes including pubkey prefixif q: THORChain deleted from the branch?
should-fix: tron API should use POST not GET
should-fix: still no |
gomesalexandre
left a comment
gomesalexandre
left a comment
There was a problem hiding this comment.
prior issues - status
| round | # | severity | issue | status | evidence |
|---|---|---|---|---|---|
| r1 | 1 | preferably-blocking | pubkey prefix heuristic fragile / trust-breaking in extractCosmosPubKeyFromTx and Tron variant |
❌ still blocking | chain/cosmos.go:52-62, chain/tron.go:56-72 - byte[0] heuristic unchanged |
| r1 | 2 | suggestion | NewCosmos/NewMaya/NewTron return error but can never error |
ℹ️ documented | still same signature; acceptable if downstream callers are already wired to handle it |
| r1 | 3 | should-fix | Cosmos 404 / 429 handling may mask server errors | code unchanged; acceptable for now | |
| r1 | 4 | suggestion | deploy/dev/01_rpc.yaml Tron public endpoint needs ops comment |
✅ fixed | config present, ops can see it's publicnode |
| r1 | 5 | should-fix | no tests for ComputeTxHash on any of the three chains |
❌ still blocking | zero *_test.go files for cosmos/maya/tron under pkg/chain/ or pkg/rpc/ |
| r1 | webpiratt | inline | add RPC config to deploy/dev/01_rpc.yaml + deploy/01_tx_indexer.yaml |
✅ fixed | 6e8864e |
round 2 findings
preferably-blocking: pubkey prefix heuristic still in place (r1 issue #1, not fixed)
Both extractCosmosPubKeyFromTx (chain/cosmos.go:52) and extractTronPubKeyFromTx (chain/tron.go:56) still use txBytes[0] == 0x02 || txBytes[0] == 0x03 as the sole signal that a pubkey is present. This is the exact ambiguity flagged in r1 - a Cosmos protobuf tx whose first length-delimited field encodes to a value starting with 0x02 or 0x03 silently gets its first 33 bytes misread as a pubkey. The comment says "we expect the caller to embed the pubkey in a metadata envelope" but there's no length prefix, no magic marker, no version byte - just a byte[0] sniff. A wrong pubkey -> wrong Sign() output -> wrong txHash -> broken payout matching. This is the core fund-safety concern and it has not been addressed since r1.
Action: use an explicit wire format. Simplest option: 1-byte magic (e.g. 0xVU) + 1-byte key length (33 or 65) + pubkey + tx bytes. That's 2 extra bytes and is unambiguous. Or better: check how the existing Thorchain / Solana / XRP indexers pass the pubkey through this same pipeline - if there's an established envelope convention already in the codebase, match it.
preferably-blocking: ComputeTxHash tests still absent (r1 issue #5)
No *_test.go added under plugin/tx_indexer/pkg/chain/ or plugin/tx_indexer/pkg/rpc/ for any of the three new chains. ComputeTxHash is the function that produces the hash used for settlement matching. A regression here = lost or mismatched payouts. At minimum one table-driven test per chain with a known (proposedTx, sigs) -> expected txHash vector. The Tron path is especially easy to test deterministically because txID = sha256(raw_data) with no external dependencies.
should-fix: tx_indexer.example.json not updated
The example config file (tx_indexer.example.json) still has no cosmos, maya, or tron entries under rpc. Any new operator following the example config to set up the indexer will have these three chains silently disabled (the if cfg.Cosmos.URL != "" guard means no error, just no indexing). Add the three entries with placeholder publicnode URLs - same pattern as the existing chains.
should-fix: gettransactioninfobyid uses GET but Tron full node expects POST
rpc/tron.go:63 does http.MethodGet against /wallet/gettransactioninfobyid?value=<hash>. The Tron full node HTTP API spec (https://developers.tron.network/reference/gettransactioninfobyid) takes a POST with body {"value": "<txid>"}, not a GET with a query param. publicnode.com may proxy and accept both, but the official API is POST-only - relying on the proxy's tolerance is fragile. Fix: use POST, send {"value": txHash} as JSON body.
suggestion: .run/CHEATSHEET.md has a hardcoded personal path
Line 1: export DYLD_LIBRARY_PATH=/Users/dev/dev/vultisig/go-wrappers/includes/darwin/ - this is a hardcoded machine-specific absolute path. Either templatize it ($GOPATH/...) or use $(go env GOPATH)/.... Minor, but it'll confuse the next dev who clones and follows the cheatsheet. Also: DYLD_LIBRARY_PATH is macOS-only and should carry a comment saying it's not needed on Linux.
q: MayaChain node endpoint - does mayanode.mayachain.info expose the standard Cosmos LCD /cosmos/tx/v1beta1/txs/<hash> path?
MayaChain is a Cosmos fork but uses custom modules. Their public node (mayanode.mayachain.info) might expose Tendermint RPC (/tx?hash=<hash>) rather than Cosmos LCD. Worth verifying the endpoint returns the tx_response.code structure before this ships - a 404 from an unsupported path would silently classify everything as pending.
CI: test job is failing on the latest run (59525396450). Build passes but the test run is red. This needs a green CI before merge regardless of the above.
verdict: REQUEST_CHANGES - two preferably-blocking items from r1 are still open (pubkey heuristic, no tests), plus new should-fixes found this round. CI also red.
3 participants
Adds transaction indexing and status checking for Cosmos, MayaChain, and TRON.
New chain indexers:
pkg/chain/cosmos.go,maya.go,tron.goNew RPC clients:
pkg/rpc/cosmos.go,maya.go,tron.goUpdated:
chains_list.go- Register new chainsconfig.go- Add RPC config entriesPart of other-chains integration. Depends on: recipes#other-chains