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 use_latest_tip query parameter to the relevant v2 endpoints #2778

Merged
merged 24 commits into from
Dec 21, 2021

Conversation

pavitthrap
Copy link
Contributor

@pavitthrap pavitthrap commented Jul 23, 2021

Description

Updated tip query parameter to the v2 RPC endpoints to accept the value of "latest".

When set to "latest", the latest tip is used - this includes the unconfirmed state, if there is any unconfirmed state.

Type of Change

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

Are documentation updates required?

Added changes to the openapi yaml file

Testing information

Added tests mostly in rpc.rs.
Added test in integrations.rs.

Copy link
Contributor

@gregorycoppola gregorycoppola left a comment

Choose a reason for hiding this comment

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

Hello. Seems like a good change but I just had a few nits about the comments, including for the tests to add some info about the logic of the test for people new to the file.

docs/rpc/openapi.yaml Outdated Show resolved Hide resolved
docs/rpc/openapi.yaml Outdated Show resolved Hide resolved
docs/rpc/openapi.yaml Outdated Show resolved Hide resolved
src/net/http.rs Outdated Show resolved Hide resolved
src/net/http.rs Outdated Show resolved Hide resolved
docs/rpc/openapi.yaml Outdated Show resolved Hide resolved
src/net/rpc.rs Outdated Show resolved Hide resolved
src/net/rpc.rs Outdated Show resolved Hide resolved
src/net/rpc.rs Show resolved Hide resolved
src/net/rpc.rs Outdated Show resolved Hide resolved
docs/rpc/openapi.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@gregorycoppola gregorycoppola left a comment

Choose a reason for hiding this comment

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

Looks good.. can you just add comments for all tests you are adding?

src/net/rpc.rs Outdated Show resolved Hide resolved
src/net/rpc.rs Outdated Show resolved Hide resolved
docs/rpc/openapi.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@gregorycoppola gregorycoppola left a comment

Choose a reason for hiding this comment

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

Thanks for all the new test comments.. I feel like I can understand it a lot better and hopefully will be the same for other readers as well!

src/net/rpc.rs Show resolved Hide resolved
src/net/rpc.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
Contributor

@gregorycoppola gregorycoppola left a comment

Choose a reason for hiding this comment

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

Thanks for the change! One small nit on the comments but LGTM.

src/net/rpc.rs Outdated Show resolved Hide resolved
src/net/http.rs Outdated Show resolved Hide resolved
src/net/http.rs Outdated Show resolved Hide resolved
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.

Overall this looks good to me @pavitthrap! I just had a couple of small but must-have testing asks. After that, I'll approve.

@jcnelson
Copy link
Member

Review is blocked on #2774

src/net/rpc.rs Outdated Show resolved Hide resolved
src/net/rpc.rs Outdated Show resolved Hide resolved
src/net/rpc.rs Outdated Show resolved Hide resolved
docs/rpc/openapi.yaml Outdated Show resolved Hide resolved
@jcnelson
Copy link
Member

This PR is looking a lot better. Thanks for adding all the thorough test coverage! Just a few minor nits to address, and then I'll approve.

@codecov
Copy link

codecov bot commented Dec 1, 2021

Codecov Report

Merging #2778 (dc34f9e) into develop (e17fcc1) will decrease coverage by 0.11%.
The diff coverage is 39.67%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2778      +/-   ##
===========================================
- Coverage    82.91%   82.79%   -0.12%     
===========================================
  Files          235      235              
  Lines       189778   190437     +659     
===========================================
+ Hits        157351   157672     +321     
- Misses       32427    32765     +338     
Impacted Files Coverage Δ
src/net/relay.rs 34.08% <0.00%> (-0.05%) ⬇️
src/net/rpc.rs 31.03% <13.48%> (-2.60%) ⬇️
src/net/http.rs 77.22% <50.68%> (-0.02%) ⬇️
src/chainstate/stacks/db/unconfirmed.rs 91.10% <55.55%> (-0.81%) ⬇️
src/clarity_vm/database/marf.rs 68.61% <83.33%> (+0.39%) ⬆️
testnet/stacks-node/src/tests/neon_integrations.rs 87.14% <91.32%> (+0.37%) ⬆️
src/clarity_vm/clarity.rs 94.02% <100.00%> (+0.14%) ⬆️
src/net/mod.rs 67.75% <100.00%> (+0.01%) ⬆️
testnet/stacks-node/src/tests/integrations.rs 40.76% <100.00%> (+0.25%) ⬆️
src/util/db.rs 80.93% <0.00%> (-0.75%) ⬇️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 194a5b8...dc34f9e. Read the comment docs.

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. I think the one thing that still stands out to me on this PR is that there's a race condition whereby the unconfirmed tip obtained from handle_load_stacks_chain_tip() can get invalidated before getting passed into maybe_read_only_clarity_tx(). However, the result of this race occurring isn't too bad -- the caller would simply receive a 404 response. I don't think this is a show-stopping problem; if it becomes a problem, the fix would be to simply load the unconfirmed chain tip hash again at the call for maybe_read_only_clarity_tx() if it got invalidated. Can you add a TODO comment describing this on handle_load_stacks_chain_tip() before merging (and open an issue for it)?

@pavitthrap pavitthrap merged commit 4fc1f28 into develop Dec 21, 2021
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.

5 participants