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

fix: remove racy endpoint PENDING_ACCEPTED_ITEMS_ENDPOINT for SESSION_STATUS_ENDPOINT #4150

Merged
merged 2 commits into from Jan 30, 2024

Conversation

joschisan
Copy link
Contributor

No description provided.

Copy link
Contributor

@dpc dpc left a 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 an improvement, but one problem with this is that the result can't be cached.

If the response included some flag if this session is closed or not the it could (when closed). Then just getting the block, without awaiting it would be entirely redundant.

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (a099fff) 58.30% compared to head (407ff05) 58.15%.
Report is 40 commits behind head on master.

Files Patch % Lines
fedimint-server/src/net/api.rs 13.33% 13 Missing ⚠️
fedimint-core/src/session_outcome.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4150      +/-   ##
==========================================
- Coverage   58.30%   58.15%   -0.15%     
==========================================
  Files         192      192              
  Lines       42757    43161     +404     
==========================================
+ Hits        24928    25100     +172     
- Misses      17829    18061     +232     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dpc
dpc previously approved these changes Jan 30, 2024
Copy link
Contributor

@dpc dpc left a comment

Choose a reason for hiding this comment

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

❤️

@elsirion
Copy link
Contributor

elsirion commented Jan 30, 2024

Do we actually need to cache this? I'd imagine to only use this endpoint for the last session we want to fetch. I think that's lower complexity overall than trying to adapt the cache to potentially non-final responses.

EDIT: otherwise we'd also need to optionally return signatures here to avoid fetching from multiple guardians, which would make this even more complex and would possibly require a new query strategy.

elsirion
elsirion previously approved these changes Jan 30, 2024
Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

LGTM

@elsirion
Copy link
Contributor

Also needs backporting since the previous version was backported too #4134 and we don't want two different APIs to live on in the next release.

@dpc don't we also have to bump the minor API version here?

@joschisan joschisan dismissed stale reviews from elsirion and dpc via 37cf9f3 January 30, 2024 08:57
@joschisan joschisan changed the title fix: remove racy endpoint PENDING_ACCEPTED_ITEMS_ENDPOINT for CURRENT_ACCEPTED_ITEMS_ENDPOINT fix: remove racy endpoint PENDING_ACCEPTED_ITEMS_ENDPOINT for SESSION_STATUS_ENDPOINT Jan 30, 2024
dpc
dpc previously approved these changes Jan 30, 2024
@dpc
Copy link
Contributor

dpc commented Jan 30, 2024

@dpc don't we also have to bump the minor API version here?

Right.

@dpc dpc enabled auto-merge January 30, 2024 18:59
@dpc dpc added this pull request to the merge queue Jan 30, 2024
Copy link
Member

@maan2003 maan2003 left a comment

Choose a reason for hiding this comment

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

approving based on past approvals

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 30, 2024
@dpc dpc added this pull request to the merge queue Jan 30, 2024
Merged via the queue into fedimint:master with commit b8ead74 Jan 30, 2024
21 checks passed
@fedimint-backports
Copy link

dpc added a commit to dpc/fedimint that referenced this pull request Jan 31, 2024
Use fedimint#4150 to avoid waiting for the final session to finish.
@elsirion
Copy link
Contributor

I get the feeling we should also document when an API was introduced and enforce that via CI somehow. Otherwise it will be fairly hard to figure out later on.

@dpc
Copy link
Contributor

dpc commented Jan 31, 2024

I get the feeling we should also document when an API was introduced and enforce that via CI somehow.

Semgrep rule to put a specially formatted comment on every api_endpoint!?

Or maybe just an extra argument to the macro... @elsirion

Edit: Let's move #4190

dpc added a commit to dpc/fedimint that referenced this pull request Jan 31, 2024
Use fedimint#4150 to avoid waiting for the final session to finish.
dpc added a commit to dpc/fedimint that referenced this pull request Feb 12, 2024
Use fedimint#4150 to avoid waiting for the final session to finish.
dpc added a commit to dpc/fedimint that referenced this pull request Feb 20, 2024
Use fedimint#4150 to avoid waiting for the final session to finish.
@joschisan joschisan deleted the current_accepted_items branch April 20, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants