Remove support for sei_getTransactionReceipt API - #3945
Conversation
This API is deprecated and no longer supported by Sei. Users should use EVM flavour APIs instead.
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3945 +/- ##
==========================================
- Coverage 59.60% 58.56% -1.04%
==========================================
Files 2331 2232 -99
Lines 200124 188773 -11351
==========================================
- Hits 119277 110563 -8714
+ Misses 69413 67685 -1728
+ Partials 11434 10525 -909
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview The Contract and integration tests switch from Reviewed by Cursor Bugbot for commit b3b9211. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped removal of sei_getTransactionReceipt: no dangling references to the removed symbols, config defaults/docker allowlists/golden and .iox fixtures stay consistent, and the JS switches to eth_getTransactionReceipt are behavior-equivalent for EVM-originated txs. Non-blocking: the unreleased CHANGELOG still tells clients to enable this method, the includeSynthetic plumbing is now hardcoded false at every call site, and a couple of tests lost the coverage they were named for.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
CHANGELOG.md:38(Unreleased → Upgrade guide, entry for #3927) still ends with "clients that know the synthetic transaction hash can enablesei_getTransactionReceipt" — this PR removes that capability in the same unreleased cycle, so that migration guidance is now wrong. Amend that entry (and/or add an entry for this PR) so the changelog matchesevmrpc/README.md, which now correctly says there is no remaining JSON-RPC method for discovering Cosmos-originated synthetic logs.- In
evmrpc/sei_legacy_test.go, the allowed-path tests were switched tosei_getCosmosTxbut the blocked-path ones still usesei_getTransactionReceipt(lines 52, 129, 383, 410, 441, 622, 673, 698, 722, 744, 767, 796, 832). Since that name is no longer inseiLegacyGatedMethods, those tests now exercise the unknown-sei_*-name fail-closed branch (duplicatingTestWrapSeiLegacyHTTP_UnknownSeiMethodBlocked) rather than the "registered gated method not in the allowlist" branch their names describe. Swapping them to a real gated name (e.g.sei_getCosmosTx) restores the intended coverage; today only the notification-batch test at line 723 still covers that branch. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
BuildSeiLegacyEnabledSet(evmrpc/sei_legacy.go:60) silently drops list entries that aren't registered gated methods, andseiLegacyMethodDisabledMessagethen tells callers of such a method "To allow this legacy method, add it to enabled_legacy_sei_apis under [evm] in app.toml" — advice that cannot work for a removed method. Operators who had uncommented the template'ssei_getTransactionReceiptline get no startup warning that the value is now inert. A startup log for unrecognizedenabled_legacy_sei_apisentries, or a distinct message for removed-vs-disabled methods, would avoid the misdirection (applies equally to the 20 names removed in #3927).
| } | ||
| } | ||
| return encodeReceipt(t.ctxProvider, t.txConfigProvider, receipt, t.keeper, block, includeSynthetic, t.globalBlockCache, t.cacheCreationMutex) | ||
| return encodeReceipt(t.ctxProvider, t.txConfigProvider, receipt, t.keeper, block, false, t.globalBlockCache, t.cacheCreationMutex) |
There was a problem hiding this comment.
[suggestion] With the sei namespace gone, includeSynthetic is now false at every production call site: here, block.go:299 (encodeReceipt), block.go:178/222 (EncodeTmBlock), and tx.go:333/398 (filterTransactions). shouldIncludeSynthetic (utils.go:308) can likewise only be reached with "eth", since NewFilterAPI is only constructed with that namespace, so filter.go's includeSyntheticReceipts is permanently false and the *wasmtypes.MsgExecuteContract branch in filterTransactions (utils.go:251) is unreachable outside tests.
Threading a constant false through five signatures is the halfway state AGENTS.md "Structural corrections" warns about — the removal reads as complete at the API layer but leaves the parameter as a convention a future caller can misread. Consider dropping the parameter (and the now-dead synthetic branches plus shouldIncludeSynthetic) here or in an immediate follow-up, rather than leaving the literals in place.
| expect(receipt).not.to.be.null; | ||
| console.log("receipt[\"blockNumber\"]", receipt["blockNumber"]); | ||
| const bn = receipt["blockNumber"]; | ||
| const bn = `0x${Number(res.height).toString(16)}`; |
There was a problem hiding this comment.
[suggestion] res.height is only set to the real inclusion height when waitForAdminTxCommit's block-walk finds the tx (contracts/test/lib.js:1007); on the logged anomalous path it stays at the -b sync CheckTx default of "0". bn then becomes 0x0, and since this test's only remaining purpose is the negative assertion expect(ethlogs.length).to.equal(0) over exactly that one block, it would pass vacuously against block 0 instead of failing. A guard such as expect(Number(res.height)).to.be.greaterThan(0) before building the filter keeps the assertion meaningful.
3 participants
This API is deprecated and no longer supported by Sei. Users should use EVM flavour APIs instead.