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

CPS-0012? | Query Layer Standardization #625

Merged
merged 21 commits into from
Sep 3, 2024

Conversation

klntsky
Copy link
Contributor

@klntsky klntsky commented Nov 27, 2023

Cardano lacks a standardized query layer. This leads to suboptimal tooling, dApp and wallet architecture.


(latest document)

@klntsky klntsky changed the title Add a draft for Query Layer Standardization CPS-????: Query Layer Standardization Nov 27, 2023
@rphair rphair added the Category: Tools Proposals belonging to the 'Tools' category. label Nov 27, 2023
@rphair rphair changed the title CPS-????: Query Layer Standardization CPS-???? | Query Layer Standardization Nov 27, 2023
@rphair
Copy link
Collaborator

rphair commented Nov 27, 2023

@klntsky it will be good to have discussion officially begin on this, during the concurrent wallet connector discussion, so I've put it on the agenda for tomorrow's CIP editors' meeting... hope you can make the same time slot again 24 hours later (https://hackmd.io/@cip-editors/77) cc @Ryun1 @Crypto2099

CPS-XXXXX/README.md Outdated Show resolved Hide resolved
Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
CPS-XXXXX/README.md Outdated Show resolved Hide resolved
rphair added a commit to rphair/CIPs that referenced this pull request Jan 27, 2024
@rphair rphair changed the title CPS-???? | Query Layer Standardization CPS-0113? | Query Layer Standardization Jan 27, 2024
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

@klntsky editors agreed ex-meeting to assign a number to this one. 🚀

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

p.s. (last review got clicked through prematurely somehow) @klntsky please also change the directory to CPS-0012 and update the link in the OP to your new document pathname.

@rphair rphair changed the title CPS-0113? | Query Layer Standardization CPS-0012? | Query Layer Standardization Jan 27, 2024
rphair added a commit that referenced this pull request Jan 30, 2024
* CIP meeting 80 updates

* adding #625 inter-meeting by undisputed request

* accidentally promoted as CIP instead of CPS
@klntsky klntsky requested a review from rphair January 30, 2024 20:41
Copy link
Collaborator

@rphair rphair 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 this is awesome and is an innovative big step forward for Cardano. 🚀

Question came up on Discord about putting some implementation code here: the editorial standard is to keep reference implementations in external repositories:

When you have your proposed JS code to validate the JSON schema please link to it from the CPS, if it's not merged by then (or we'll do another PR later), or from a solution CIP if it emerges & seems more appropriate there.

Also let's remember to update this link in your document to a merged CIP when that becomes possible... since that may happen in the same time frame: 🤓

Ryun1 pushed a commit to Ryun1/CIPs that referenced this pull request Mar 6, 2024
…foundation#750)

* CIP meeting 80 updates

* adding cardano-foundation#625 inter-meeting by undisputed request

* accidentally promoted as CIP instead of CPS
Ryun1 and others added 3 commits April 18, 2024 13:12
Co-authored-by: Vladimir Kalnitsky <klntsky@gmail.com>
Co-authored-by: Vladimir Kalnitsky <klntsky@gmail.com>
CPS-0012/README.md Outdated Show resolved Hide resolved
CPS-0012/README.md Outdated Show resolved Hide resolved
@klntsky
Copy link
Contributor Author

klntsky commented May 13, 2024

@rdlrt

different query layers may use different backend models/DB designs (while majority rely on cardano-db-sync, CF's ledger-sync seems to be maturing well and could offer alternatives), the output results could differ depending on the data model (which would have performance implications).

I don't see a lot of risks there. At least for common RDBMS, data models can be altered via migrations if performance is lacking for new endpoints (e.g. due to absence of appropriate indices). This performance improvement is and should be an iterative process, but I don't think that we should put too much attention there while building a standard, unless there are concerns that the queries we may want to provide are impossible to implement efficiently (I doubt that would be the case, the relations between different data are very simple).

For instance - at Koios - we use option to query bulk objects at once, reducing number of calls that require to be made to query layer. This might become a bottleneck (either performance or cost) for some providers. Similarly, there are endpoints that try to provide a lot of information against a single call leaving the option to users. The point being these become preferences that might show alternate paths taken leading to different results (and different consumer base prefer those different solutions).

Batching specifically could (and I think it should) be included in the spec. For example, just like ETH's RPC allows to get info in batches using some endpoints. The limits may even vary between providers - e.g. Ethereum-like RPC providers set their own limits for eth_getLogs batch size.

Of course providers may offer features other than batching - if they are non-standard, they should be just scoped separately, so that the users will understand that they would sign for a vendor lock-in by using them. Different libraries would still be able to wrap these endpoints - it's just that the "basic" set of functionality will be fixed (which is an improvement over not having a standardized API endpoint set).

As it happens typically in engineering, wanting to create a standard ends up with yet-another-flavour - which is not something providers/consumers/libraries would prefer, unless it is consistent right to the core from cardano's ledger itself from consumption pov - so that all the tooling around (dbsync, ogmios/kupo , ledger-sync, carp, oura, mafoc ...) is consistent accordingly (easier for middleware to be as close as possible to the data layer).

We have discussed this in one of the calls.

Existing code will not go away, just as existing query layer providers will have to maintain compatibility with their clients' code for at least some time. However, as consumers would slightly prefer to avoid vendor-locking themselves, there would be a motivation to use a "standardized" implementation.

For instance, carp got created because the data model of DB Sync was too opinionated.

Could you please point out speicific design choices that were too opinionated/inconvenient?

@rdlrt
Copy link

rdlrt commented May 14, 2024

Could you please point out speicific design choices that were too opinionated/inconvenient?

Not design choices, but the data model (or formats) - that was mentioned in the introductory blog from carp ( here ).

Copy link
Collaborator

@Ryun1 Ryun1 left a comment

Choose a reason for hiding this comment

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

A few final comments cleaning up grammar/spelling
Beyond these small comments, I believe this CPS is good to be merged

CPS-0012/README.md Outdated Show resolved Hide resolved
CPS-0012/README.md Outdated Show resolved Hide resolved
CPS-0012/README.md Outdated Show resolved Hide resolved
CPS-0012/README.md Outdated Show resolved Hide resolved
CPS-0012/README.md Outdated Show resolved Hide resolved
CPS-0012/README.md Outdated Show resolved Hide resolved
CPS-0012/README.md Outdated Show resolved Hide resolved
CPS-0012/README.md Outdated Show resolved Hide resolved
CPS-0012/README.md Outdated Show resolved Hide resolved
@rphair
Copy link
Collaborator

rphair commented Aug 20, 2024

@klntsky we are trying to merge this so please respond to @Ryun1's #625 (review) - I hate to mark something Waiting for Author while we are so close to the "finish line" and with only such trivial changes left to make.

But we do need to make those final spelling/format corrections before merge (I'll pass it myself after you have made those changes). True, the editors could commit those suggestions (and we might, rather than leave this PR stalled) it would be better process for this to be done by you... I'll change it to Last Check as soon as this is done. 🙏

@rphair rphair added the State: Waiting for Author Proposal showing lack of documented progress by authors. label Aug 20, 2024
klntsky and others added 2 commits August 20, 2024 21:53
Fix phrasing

Co-authored-by: Ryan <44342099+Ryun1@users.noreply.github.com>
Co-authored-by: Ryan <44342099+Ryun1@users.noreply.github.com>
@klntsky
Copy link
Contributor Author

klntsky commented Aug 20, 2024

@rphair Thanks! Commited everything.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

good stuff @klntsky - changing status to Last Check & so we should be able to merge this at the next CIP meeting.

@rphair rphair added State: Last Check Review favourable with disputes resolved; staged for merging. and removed State: Waiting for Author Proposal showing lack of documented progress by authors. labels Aug 20, 2024
@rphair
Copy link
Collaborator

rphair commented Sep 3, 2024

@klntsky we ran out of time at a really packed meeting but @Ryun1 who has been involved in this one for a while should be able to confirm that this is ready to merge.

@rphair rphair requested a review from Ryun1 September 3, 2024 17:04
Copy link
Collaborator

@Ryun1 Ryun1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@rphair rphair merged commit 7067170 into cardano-foundation:master Sep 3, 2024
@rphair rphair removed the State: Last Check Review favourable with disputes resolved; staged for merging. label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Tools Proposals belonging to the 'Tools' category.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants