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: RPC endpoint /v2/traits/<contract-info>/<trait-info> #2498

Merged
merged 4 commits into from
Mar 11, 2021

Conversation

pavitthrap
Copy link
Contributor

Added a RPC endpoint to determine if a given trait is implemented in a contract, either explicitly or implicitly.
Fixes #2382

Description

For some background, a trait is explicitly implemented in a contract if the contract has impl-trait <trait info>. Implicit implementation means a contract implements the traits without the declaration above.

Notes

  • I made trait_id (type TraitIdentifier) a field in HttpRequestType::GetIsTraitImplemented, even though the GET request independently passes in a trait name, trait contract address, and trait contract name. I did this because I thought it would be cleaner to parse the API path at a higher level, and pass down consolidated information (in the form of the struct TraitIdentifier. However, this is not consistent with how the contract data is passed (which is passed separately as contract address and contract name), so let me know if I should avoid consolidating the trait info until the function handle_get_is_trait_implemented in src/net/rpc.rs.
  • For implicitly implemented traits, I have to check for trait compliance. However, since I am using a read only connection to the clarity database, I am not saving the results of this computation. If I were to save the computation, I would add the implicitly implemented trait by calling contract_analysis.add_implemented_trait, and save the updated contract analysis by calling set_metadata, which is a method of ClarityDatabase. Let me know if I should open a RW connection to the database instead, and commit this information.

Type of Change

  • New feature
  • Bug fix
  • API reference/documentation update
  • Other

Does this introduce a breaking change?

List the APIs or describe the functionality that this PR breaks.
Workarounds for or expected timeline for deprecation

Are documentation updates required?

Want to update the RPC endpoint documentation here

Testing information

Ran cargo test.

@jcnelson jcnelson self-requested a review March 8, 2021 16:20
@kantai
Copy link
Member

kantai commented Mar 8, 2021

Thanks for this PR, @pavitthrap! One note before I take a closer look at the PR: this should be opened against develop. Generally speaking we follow something like a git-flow process for PRs, where features and other non-hotfix PRs should be based off of develop, and hotfixes are based off of master.

@pavitthrap pavitthrap force-pushed the feat/expose-implemented-traits branch from ce26d03 to 9f70421 Compare March 8, 2021 23:45
@pavitthrap pavitthrap changed the base branch from master to develop March 8, 2021 23:45
src/net/http.rs Outdated Show resolved Hide resolved
src/net/rpc.rs Outdated Show resolved Hide resolved
src/net/rpc.rs Outdated Show resolved Hide resolved
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @pavitthrap! I just had a few style nits. And since this PR adds a new RPC endpoint, you should update two places in the PR:

  1. Add a brief changelog entry under the [Unreleased] section in CHANGELOG.md
  2. Add an entry for the new endpoint to the OpenAPI spec ./docs/rpc/openapi.yaml

@pavitthrap pavitthrap force-pushed the feat/expose-implemented-traits branch from 9f70421 to ea6e0c6 Compare March 10, 2021 18:26
@pavitthrap pavitthrap requested a review from kantai March 10, 2021 18:26
docs/rpc/openapi.yaml Outdated Show resolved Hide resolved
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

LGTM! Just one comment on the endpoint's tag.

A note for the future: it'd be better not to do a force-push to update the PR once you've already opened the PR. It makes checking for resolution of comments harder because the reviewer can't see the specific changes that were made to address them.

@pavitthrap
Copy link
Contributor Author

LGTM! Just one comment on the endpoint's tag.

A note for the future: it'd be better not to do a force-push to update the PR once you've already opened the PR. It makes checking for resolution of comments harder because the reviewer can't see the specific changes that were made to address them.

Ah got it. It seemed like the only way here since I needed to rebase to get the openAPI changes, but I'll avoid it in other scenarios.

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @pavitthrap

@pavitthrap pavitthrap merged commit d6c54d6 into develop Mar 11, 2021
@pavitthrap pavitthrap deleted the feat/expose-implemented-traits branch March 17, 2021 18:21
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] expose implemented traits via interface RPC API
4 participants