-
Notifications
You must be signed in to change notification settings - Fork 925
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(state/gateway)!: remove SubmitTx endpoint from RPC and Gateway #3391
refactor(state/gateway)!: remove SubmitTx endpoint from RPC and Gateway #3391
Conversation
39993e8
to
b33aa28
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3391 +/- ##
==========================================
+ Coverage 44.83% 45.48% +0.65%
==========================================
Files 265 274 +9
Lines 14620 15397 +777
==========================================
+ Hits 6555 7004 +449
- Misses 7313 7585 +272
- Partials 752 808 +56 ☔ View full report in Codecov by Sentry. |
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.
My fav type of PRs - deletions only! 🚀 Less is more, right?
829fda9
to
67a7d53
Compare
This PR removes the
state.SubmitTx
endpoint from the RPC, and thesubmit_tx
endpoint from the gateway.Related: #3348
Rationale
This endpoint was never fully documented, and no usage examples exist.
When creating usage examples after a discussion in #3345 , we realized the endpoint is not very useful at all. In order to use it, you need to construct a transaction, importing app, tendermint... all to then proxy it to an app instance via node anyways. Here is a snippet from @jcstein in the docs tutorial for submitting data.
The example for
celestia-openrpc
/celestia-node
would essentially be exactly the same, except we build a rpc connection to node instead of a grpc to app. I think it is clear from this example why SubmitTx is too much of a hassle to use, and if users do need this functionality for their use-case, it is likely a sign that they should be interacting with app directly instead anyways.To verify this, @Bidon15 reached out to teams to get feedback on this decision. All teams confirmed that the endpoint is not being used and not needed.
We support enough transaction types in the state module. If users need more functionality, we should either add the granularity to the current methods or add specific methods for specific transaction types/use cases. Dropping down a level to SubmitTx should never be needed if our API is good enough.
Example (using app conn)