-
Notifications
You must be signed in to change notification settings - Fork 165
[AccessAPI] Add grpc and rest endpoints for schedule transaction endpoints #1638
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
base: master
Are you sure you want to change the base?
Conversation
openapi/access.yaml
Outdated
/system_transactions/{id}: | ||
get: | ||
summary: Get a System Transaction by ID. | ||
description: Get a system transaction body by the provided transaction ID. |
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.
I think it's probably worth bringing up a concern I had about system TX ID being used as an index.
AFAIK system TXns not inherently unique due (ex, no unique params) to empty proposer key, so I am unsure whether we can use the ID hash as a valid index.
Assuming that we could use the TX hash as a unique index, then we may not need this endpoint altogether. Could we just serve these results from /transactions/{id}
to simplify?
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.
AFAIK system TXns not inherently unique due (ex, no unique params) to empty proposer key, so I am unsure whether we can use the ID hash as a valid index.
it's valid for getting the transaction (since it's a hash of the tx body), but not the result. for the result, a blockID is required.
Assuming that we could use the TX hash as a unique index, then we may not need this endpoint altogether. Could we just serve these results from /transactions/{id} to simplify?
I think we could serve it using /transactions/{id}
, as well as /transaction_results/{transaction_id}
. The complication for the latter is that we would require the blockID for system tx since there could be many, but we do not require it for regular tx. This would make the API a bit messy.
these endpoints were originally created to support the flow-cli. maybe they aren't needed?
onflow/flow-cli#1123
looking back at that PR, I don't totally understand why the GetTransactionsByBlockID
and GetTransactionResultsByBlockID
weren't sufficient.
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.
Ah, yeah great point - I did not consider this yeah I guess the /transactions/{id}
would be completely valid.
Honestly IMO we should just use the existing endpoints & add an optional blockID flag for the block ID in the results endpoint.
One flag would be that from the REST API we currently have no equivalent of the GetTransactionsByBlockID
/ GetTransactionResultsByBlockID
RPC. The only way to get the transactions/results for a blocks in general is via GetBlock
=> query collections to get TX_IDs => query tx/result AFAIK.
Without this endpoint/RPC REST clients will need a way to get info on what system transactions exist for a block. I'd figure this could just be added to the existing GET /blocks/{id}
response, e.g.
systemTransactionsIds: ["123"] // Or more verbose if it makes sense?
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.
Honestly IMO we should just use the existing endpoints & add an optional blockID flag for the block ID in the results endpoint.
blockID is already optional. we will just need to require it if the request was for a static system tx. it wouldn't have to be required for user tx or scheduled transactions. though if a user has a scheduled transaction's txID, the it must know or be able to know the block it was in.
One flag would be that from the REST API we currently have no equivalent of the GetTransactionsByBlockID / GetTransactionResultsByBlockID RPC. The only way to get the transactions/results for a blocks in general is via GetBlock => query collections to get TX_IDs => query tx/result AFAIK.
these could be added. what's your sense on priority for these?
Without this endpoint/RPC REST clients will need a way to get info on what system transactions exist for a block. I'd figure this could just be added to the existing GET /blocks/{id} response, e.g.
I'd prefer to not include this in blocks. if we did, probably adding a way to articulate the system collection and make it queryable.
@jribbink I updated the openapi spec to remove the system and schedule tx endpoints, and integrate them into the main |
Relates: 7830
Description
Adds these gRPC endpoints:
GetScheduledTransaction
- returns the transaction body of the scheduled transaction by callbackIDGetScheduledTransactionResult
- returns the transaction result of the scheduled transaction by callbackIDand these REST endpoints:
/scheduled_transactions/{id}
- returns the transaction body of the scheduled transaction by callbackID/scheduled_transaction_results/{id}
- returns the transaction result of the scheduled transaction by callbackIDalso adding these REST endpoints which were missing
/system_transactions/{id}
- returns the system transaction body for the requested transaction ID/system_transaction_results/{transaction_id}
- returns the system transaction result for the requested transaction IDFor contributor use:
master
branchFiles changed
in the Github PR explorer