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.

Sign up for GitHub

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

Jump to bottom

Support queryLatestState in ViewService#auctions #1035

Merged
merged 8 commits into from
May 8, 2024
Merged

Support queryLatestState in ViewService#auctions #1035

merged 8 commits into from
May 8, 2024

Conversation

jessepinho
Copy link
Contributor

@jessepinho jessepinho commented May 4, 2024 •
edited
Loading

🚨 Note that my local devnet does not actually respond when I call the auction's QueryService#auction_state_by_id RPC method. Not sure why, but it's still probably OK to merge this, since none of our code uses queryLatestState yet.

AuctionsRequest has an optional queryLatestState boolean property that, when passed with true, indicates that we should call out to a fullnode to get the auction's latest state. This PR adds support for that.

In this PR

  • Create a new AuctionQuerier class and add it to the RootQuerier.
  • When queryLatestState is passed, call out to the AuctionQuerier. Take the returned state and add it to the ViewService#auctions response.

Closes #980

@jessepinho jessepinho force-pushed the jessepinho/auctions-rpc-method-web-980 branch from 5decab2 to 4a3ec0d Compare May 7, 2024 00:42
Base automatically changed from jessepinho/auctions-rpc-method-web-980 to main May 7, 2024 00:48
@jessepinho jessepinho force-pushed the jessepinho/auctions-rpc-query-latest-state-web-980 branch 3 times, most recently from 83fd279 to 6aa6869 Compare May 7, 2024 16:52
@jessepinho jessepinho force-pushed the jessepinho/auctions-rpc-query-latest-state-web-980 branch from 6aa6869 to b82e4c4 Compare May 7, 2024 17:32
@jessepinho jessepinho changed the base branch from main to jessepinho/auctions-rpc-include-inactive-web-980 May 7, 2024 17:33
@jessepinho jessepinho changed the title WIP: Support queryLatestState in ViewService#auctions Support queryLatestState in ViewService#auctions May 7, 2024
@jessepinho jessepinho marked this pull request as ready for review May 7, 2024 18:06
@jessepinho jessepinho force-pushed the jessepinho/auctions-rpc-query-latest-state-web-980 branch from d86d87a to 7585700 Compare May 7, 2024 20:23
@jessepinho jessepinho force-pushed the jessepinho/auctions-rpc-query-latest-state-web-980 branch from 7585700 to 590f69c Compare May 7, 2024 20:26
@jessepinho jessepinho marked this pull request as draft May 8, 2024 01:01
TalDerei
TalDerei requested changes May 8, 2024
View reviewed changes
Copy link
Contributor

@TalDerei TalDerei left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should be making an additional RPC request here. The point of query_latest_state was to make it easier for clients to get the latest state without making additional RPC calls. If include_inactive in AuctionsRequest is set to true, then AuctionsResponse will include the latest auction state. cc @erwanor

@erwanor
Copy link
Member

erwanor commented May 8, 2024

@TalDerei That's right but afaict that's what this PR does? no? why not? it setup a view service implementation that offer an option for clients to request the latest auction state, which is what we want. This makes it easy to improve later on because every query service call is mediated by the user's view server from the get go

@erwanor
Copy link
Member

erwanor commented May 8, 2024 •
edited
Loading

This LGTM as far as the existing work done feel free to ping again for more review, @TalDerei re: include_inactive: that's not necessarily the case that we will return concrete auction state if that flag is set.

The view service implementation contract is like this:

  • include_inactive -> include note records for auctions that are closed or withdrawn
  • query_latest_state -> somehow return concrete auction state / DEX positions (in practice, via rpc calls to the auction/dex query services)

It's completely valid to ask for include_inactive with query_latest_state set to false

@TalDerei
Copy link
Contributor

TalDerei commented May 8, 2024

If include_inactive in AuctionsRequest

oops typo here, meant If query_latest_state in AuctionsRequest is set to true.

@TalDerei TalDerei self-requested a review May 8, 2024 17:06
TalDerei
TalDerei approved these changes May 8, 2024
View reviewed changes
Copy link
Contributor

@TalDerei TalDerei left a comment

Choose a reason for hiding this comment

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

@erwanor thanks for the clarification, this makes sense! I re-reviewed the PR and the logic seems to be all there

@jessepinho jessepinho marked this pull request as ready for review May 8, 2024 17:37
@jessepinho jessepinho merged commit eca90c4 into jessepinho/auctions-rpc-include-inactive-web-980 May 8, 2024
6 checks passed
@jessepinho jessepinho deleted the jessepinho/auctions-rpc-query-latest-state-web-980 branch May 8, 2024 18:27
jessepinho added a commit that referenced this pull request May 8, 2024
@jessepinho
Handle includeInactive in ViewService#auctions (#1047)
fea6900 
* Store auction state in the database

* Rename state -> seqNum

* Add test re: upserting seqNum to be safe

* Store the sequence number in the block processor

* Support includeInactive

* Refactor

* Handle Dutch auction withdrawals in the block processor

* Bump IDB version

* Support `queryLatestState` in `ViewService#auctions` (#1035)

* Support queryLatestState

* Simplify

* Remove unused method

* Change method signature

* Rework a bit

* Remove unused import

* Account for the fullnode returning a DutchAuction, not a DutchAuctionState

* Fix type name check

* UI for ending auctions (#1060)

* Build UI to end an auction

* Fix layout issue

* Tweak symbols

* Reload auctions after scheduling or ending an auction

* Save auction metadata when ending an auction

* Rename helper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TalDerei TalDerei TalDerei approved these changes

@erwanor erwanor erwanor approved these changes

@grod220 grod220 Awaiting requested review from grod220

@turbocrime turbocrime Awaiting requested review from turbocrime

@Valentine1898 Valentine1898 Awaiting requested review from Valentine1898

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jessepinho @erwanor @TalDerei