Skip to content
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

feat: added view_receipt_record rpc endpoint #266

Merged

Conversation

frolvanya
Copy link
Collaborator

Here's an example of calling view_receipt_record:
image

closes #264

@khorolets khorolets self-requested a review June 1, 2024 07:47
Copy link
Member

@khorolets khorolets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job indeed! I left a few change requests, but please wait for @kobayurii review before rushing on it. Thank!

rpc-server/src/modules/receipts/methods.rs Outdated Show resolved Hide resolved
rpc-server/src/modules/receipts/methods.rs Outdated Show resolved Hide resolved
rpc-server/src/metrics.rs Outdated Show resolved Hide resolved
@frolvanya
Copy link
Collaborator Author

Thanks for a review! I guess I need to pay more attention to delete all debug leftovers. I've fixed first two issues and I'll wait for @kobayurii review, before pushing changes. However, I still don't know if it's fine to return non Response type from RPC endpoint. Can someone tell me if it's a good practice:

#[derive(serde::Serialize, serde::Deserialize, Debug)]
pub struct ReceiptRecord {

/// Fetches a receipt record by it's ID
#[cfg_attr(feature = "tracing-instrumentation", tracing::instrument(skip(data)))]
pub async fn view_receipt_record(
data: Data<ServerContext>,
Params(params): Params<serde_json::Value>,
) -> Result<readnode_primitives::ReceiptRecord, RPCError> {

Copy link
Collaborator

@kobayurii kobayurii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Great job! I left a little comment. Also, I noticed that you created a pull request in the main branch. Please reopen the pull request for the develop branch. develop is the main branch for development. Also don't forget to fix the checker.

@khorolets
Copy link
Member

However, I still don't know if it's fine to return non Response type from RPC endpoint. Can someone tell me if it's a good practice:

#[derive(serde::Serialize, serde::Deserialize, Debug)]
pub struct ReceiptRecord {

/// Fetches a receipt record by it's ID
#[cfg_attr(feature = "tracing-instrumentation", tracing::instrument(skip(data)))]
pub async fn view_receipt_record(
data: Data<ServerContext>,
Params(params): Params<serde_json::Value>,
) -> Result<readnode_primitives::ReceiptRecord, RPCError> {

While it's not perfect either, I would suggest following what @kobayurii did for the other custom method we have here

https://github.com/near/read-rpc/blob/develop/rpc-server/src/modules/state/mod.rs

I am not a fan of duplicating structures, but I would ask you to have the Resuest/Response structures defined in the modules/receipts/mod.rs

Feel free to implement From<ReceiptRecord> or TryFrom for your request/response structures. And drop the serde derives from the original structure.

My thoughts come from the idea of having dedicated structures for (again) request/response. Having From or TryFrom from the internal structure would alert us if we change something in the future.

Thank you for drawing my attention to this question 🙏

@khorolets khorolets changed the base branch from main to develop June 3, 2024 07:39
@khorolets
Copy link
Member

@frolvanya and please update https://github.com/near/read-rpc/blob/main/docs/CUSTOM_RPC_METHODS.md with info about the new method you've added. Thank you.

@frolvanya frolvanya force-pushed the feat/added-view_receipt_record-endpoint branch from ac8f4f2 to 88491b0 Compare June 3, 2024 14:45
@frolvanya
Copy link
Collaborator Author

Resuest/Response structures

We need only Response struct, since we can use RpcReceiptRequest as a Request

Copy link
Member

@khorolets khorolets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! Thank you 🙏

@khorolets khorolets dismissed kobayurii’s stale review June 3, 2024 15:48

I've already changed the target branch to develop as @kobayurii requested

@khorolets khorolets merged commit dacba79 into near:develop Jun 3, 2024
3 checks passed
kobayurii added a commit that referenced this pull request Jun 5, 2024
* fix(near-state-indexer): Fix the TypeError in communication with Redis leading to spam of warnings (#254)

* (rpc_server): metrics refactoring and improvements (#255)

* merics reffactoring and imrovements

* metrics improvements according pr comments

* improvement tracing instrument logs (#257)

* (database): Implement metrics to count requests for each table (#259)

* Implement metrics to count request for each database table

* remove logic to fetch all state before contract call

* add issue to implement metrics for postgres

* rename mentric DATABASE_QUERIES to DATABASE_READ_QUERIES

* refactor(rpc-server): Tweaks and clean up around query.call_function method implementation (#258)

* refactor(rpc-server): Tweaks and clean up around query.call_function method implementation

* Follow the clippy suggestion

* Drop redundant pieces of code

* fix tx different results (#261)

* Metrics(rpc-server): Extend metrics (#263)

* extend metrics to collect block category

* fmt

* code improvement according git comments

* Revert "fix tx different results" (#265)

This reverts commit 2e4dc30.

* feat: added `view_receipt_record` rpc endpoint (#266)

* feat: added `view_receipt_record` rpc endpoint

* chore: removed leftovers

* feat: used appropriate metrics

* feat: added proper response structure

* docs(view_receipt_record): added new custom method

* hook for poolv1.near (#267)

* chore: relese v0.2.9 (#268)

* proxy next_light_client_block to regular rpc (#269)

* fix changelog.md (#270)

---------

Co-authored-by: Bohdan Khorolets <[email protected]>
Co-authored-by: Ivan Frolov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a JSON-RPC method to get a parent transaction hash for a given receipt ID
3 participants