exp/services/soroban-rpc: Add getTransactionStatus and sendTransaction json rpc methods #4609
Conversation
There was a problem hiding this comment.
Overall looks good. Beside the notes added, I have one more request:
can you add a Stop()/Shutdown() that would cancel the context and block until all the child go routines are done ?
i.e. use sync/workgroup + child context so you could shut it down.
The goal here is to allow deterministic cleanup.
| type TransactionStatusResponse struct { | ||
| ID string `json:"id"` | ||
| Status string `json:"status"` | ||
| Result *horizon.Transaction `json:"result"` |
There was a problem hiding this comment.
This should probably follow the soroban-rpc api docs from confluence instead: https://stellarorg.atlassian.net/wiki/spaces/PJC/pages/4402151440/soroban-rpc+API+Docs#getTransactionStatus-(Meridian-or-use-Horizon)
There was a problem hiding this comment.
Also, error handling here?
There was a problem hiding this comment.
Also, error handling here?
+1, if not a big stretch to populate error as optionally mentioned in the api docs, seems like could help diagnostics for clients.
There was a problem hiding this comment.
ok I will try to follow that format but I will keep Result to have type *horizon.Transaction for now. Once we're able to submit soroban transactions through horizon I will update the TransactionStatusResponse to include the the smart contract return value. We will also need stellar/stellar-core#3543 to be merged to include the smart contract return value
| return TransactionStatusResponse{}, err | ||
| } | ||
| } else { | ||
| return TransactionStatusResponse{ |
There was a problem hiding this comment.
could p.deletePendingEntry(request.Hash) be done at this point as optimization, since tx status is known now, to keep the pending queue size down, rather than letting it wait longer for the expired loop to purge it.
| } | ||
| txHash := hex.EncodeToString(hash[:]) | ||
|
|
||
| p.lock.Lock() |
There was a problem hiding this comment.
in general, and not suggesting for now, unless you saw value in it and not a stretch, has a context-aware lock been considered for usage, like go-lock with tryLockWithContext(ctx) just curious with advanced threading, potential for getting locked up and not honoring the callers ctx.
There was a problem hiding this comment.
@sreuland where do we have a use case where we use tryLockWithContext ? I'm familiar with the concept, but always found it too of a headache to implement.
There was a problem hiding this comment.
I was thinking on ln 109, if there were a mis-matchted lock/unlock at some point, then ensuing caller requests would get stuck there on the Lock(), also if the caller defined a ctx with timeout shorter than the http client timeout(is configurable, I think is default of 10s) and passes that in here, could be another potential way that caller's context is ignored if the http response takes longer.
There was a problem hiding this comment.
I generally agree with your observation here; as you've noted, in this case the lock is taken only for a short amount of time, and never for a blocking operation ( therefore, I can't see any concrete potential issue here )
| func TestDeleteExpiredTransaction(t *testing.T) { | ||
| ttl := time.Minute | ||
| proxy := NewTransactionProxy( | ||
| nil, |
There was a problem hiding this comment.
great test coverage btw, from this unit test layer, noticed may have been able to mock out HTTP in horizonclient.Client, and possibly exercise much of the new code paths, rather than system integration tests later, just thinking out loud, not suggesting change.
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
Add json rpc methods for sending a transaction and querying a transaction status. This is implemented by proxying to the horizon txsub endpoint and the horizon transaction detail endpoint.
Why
#4595
#4595
Known limitations