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

Add sat recursive endpoints with index and pagination #2680

Merged
merged 20 commits into from
Nov 23, 2023

Conversation

raphjaph
Copy link
Collaborator

@raphjaph raphjaph commented Nov 15, 2023

This PR takes the discussion from #2452 and implements the following recursive endpoints:

/r/sat/:sat -> get the first 100 inscription ids on a sat (ascending reinscription order)
/r/sat/:sat/:page -> get 100 inscription ids on sat paginated (ascending reinscription)

These two endpoints return:

{
  "ids": [ 
    "c3a05e88abd51708954ce562389478028567184e32fb96ab8d07b672989155b8i0", 
    "c3a05e88abd51708954ce562389478028567184e32fb96ab8d07b672989155b8i1"
  ],
  "page": 2,
  "more": false,
}

/r/sat/:sat/at/:index -> get a single inscriptions id on a sat by index (-1 is latest, 0 is first inscription)

This returns:

{  
  "id": "c3a05e88abd51708954ce562389478028567184e32fb96ab8d07b672989155b8i0", 
}

:sat can be any represenation of a sat including number, degree or letter notation.

Originally this PR would allow to use any sat notation like degree or percentile. For now we only accept the sat number.
#2732

Copy link
Contributor

@elocremarc elocremarc left a comment

Choose a reason for hiding this comment

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

Should shorten route name to save bytes.

tests/server.rs Outdated Show resolved Hide resolved
src/subcommand/server.rs Outdated Show resolved Hide resolved
tests/server.rs Outdated Show resolved Hide resolved
src/subcommand/server.rs Outdated Show resolved Hide resolved
@raphjaph raphjaph marked this pull request as ready for review November 22, 2023 20:33
src/subcommand/server.rs Outdated Show resolved Hide resolved
src/subcommand/server.rs Outdated Show resolved Hide resolved
src/subcommand/server.rs Outdated Show resolved Hide resolved
src/subcommand/server.rs Outdated Show resolved Hide resolved
src/subcommand/server.rs Outdated Show resolved Hide resolved
src/subcommand/server.rs Outdated Show resolved Hide resolved
tests/server.rs Show resolved Hide resolved
src/index.rs Outdated Show resolved Hide resolved
@raphjaph raphjaph changed the title Sat endpoint with pagination Add sat recursive endpoints with index and pagination Nov 22, 2023
@elocremarc
Copy link
Contributor

Why are we doing /r/sat/:sat/at/:index
at is to many bytes its not needed and why are you disregarding my PR? This pretty much kills my entire motivation to help at all on this repo kinda a little frustrated right now to be honest because I worked hard on that PR and you just go do your own.

@casey
Copy link
Collaborator

casey commented Nov 23, 2023

Why are we doing /r/sat/:sat/at/:index at is to many bytes its not needed

We have three routes: Get inscriptions on a sat, get inscriptions on a sat with page index, and get a single inscription on a sat by index.

We were discussing names today, and the three names that we thought were best are:

/sat/:sat # get first page of inscriptions on a sat
/sat/:sat/:page # get `page` of inscriptions on a sat
/sat/:sat/at/:index # get a single sat at an index

We thought /sat/:sat/at/:index was a less ambiguous option.

and why are you disregarding my PR? This pretty much kills my entire motivation to help at all on this repo kinda a little frustrated right now to be honest because I worked hard on that PR and you just go do your own.

I understand your frustration, but opening a PR is not a guarantee that it will be merged, and not a guarantee that someone else won't open and merge a PR for the same functionality. Sometimes PRs would be hard to get into mergable shape, so we will write our own, when we want to get the feature out quickly.

Raph and I are super rate limited, we have an infinite amount of stuff to do, and very limited bandwidth to do it. I'd like to do better with this in the future. I don't know what the best way to handle this is, in general. We want to welcome contributions and contributors, but we have to prioritize getting ord into the best shape possible.

@elocremarc
Copy link
Contributor

We thought /sat/:sat/at/:index was a less ambiguous option.

Isn't that what documentation is for it uses zero blockspace? We had a lot of discursion of reducing the byte limit the sat number already takes up quite a bit of characters.

Sometimes PRs would be hard to get into mergable shape, so we will write our own, when we want to get the feature out quickly.

Understood glad I was able to help move this discussion along opening a PR.

We want to welcome contributions and contributors, but we have to prioritize getting ord into the best shape possible.

How do we get people up to speed so their PRs can be closer to being mergable? Some of it might take a bit of hand holding and a bit of bandwidth however the more people we get familiar with the codebase the better this will become. Streams are a big help.

@elocremarc
Copy link
Contributor

elocremarc commented Nov 23, 2023

        .route("/r/sat/:sat", get(Self::sat_inscriptions))
        .route("/r/sat/:sat/:page", get(Self::sat_inscriptions_paginated))
        .route("/r/sat/:sat/at/:index", get(Self::sat_inscription_at_index))

I see now that /r/sat/:sat/:index would clash with the pagination route hence the /at/. Is the first route needed? Can we not bring back the sat info to get the output of the sat with that one? Because you can get at the 100 with the page route however this kind of closes off having sat info in the future if we ever want to add that. I think knowing the output(satpoint) is very desirable and non deterministic.

@casey
Copy link
Collaborator

casey commented Nov 23, 2023

We were thinking of adding something like /r/location/:sat to get the current location of a sat.

@elocremarc
Copy link
Contributor

elocremarc commented Nov 23, 2023

We were thinking of adding something like /r/location/:sat to get the current location of a sat.

/r/info/:sat Do we not want to add rarity or date? Can you find rarity with a JS library? Date could be cool to make a gallery of older sat inscriptions. Info is also shorter. I know we don't want to make an API however for web scrapers so I see the balance with that decision of limiting the info returned.

@casey
Copy link
Collaborator

casey commented Nov 23, 2023

Rarity and date can both be calculated with a JS library, so I don't think we'd add them.

@elocremarc
Copy link
Contributor

elocremarc commented Nov 23, 2023

We were thinking of adding something like /r/location/:sat to get the current location of a sat.

LGTM then! location can be added later I think. Or we could maybe just get that info with the /r/inscription/:id recursive route. See #2628

Copy link
Collaborator

@casey casey left a comment

Choose a reason for hiding this comment

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

LGTM!

@raphjaph raphjaph merged commit 61fbdbd into master Nov 23, 2023
6 checks passed
@raphjaph raphjaph deleted the sat-endpoint-with-pagination branch November 23, 2023 02:07
thedoublejay pushed a commit to sadoprotocol/ord that referenced this pull request Nov 27, 2023
* Fix lost sats bug (ordinals#2666)

* Add Hindi version of handbook (ordinals#2648)

* Remove Index::index_block_inscription_numbers (ordinals#2667)

* Hide protocol inscriptions (ordinals#2674)

* Don't color links in headers (ordinals#2678)

* Add inscription charms (ordinals#2681)

* Group rune server tests (ordinals#2685)

* Add inscription compression (ordinals#1713)

* Fix media table formatting (ordinals#2686)

* Update schema version for charms (ordinals#2687)

* Fix unbound outpoint server error (ordinals#2479)

* Add binary media type (ordinals#2671)

* Clean up install.sh (ordinals#2669)

* Add /collections Page (ordinals#2561)

* Preview font inscriptions (ordinals#2692)

* Only load used language highlight module in code preview (ordinals#2696)

* Only try to create the database if it wasn't found (ordinals#2703)

* Move postage into batch file (ordinals#2705)

* Add destination field to batch (ordinals#2701)

* Use sequence numbers database keys (ordinals#2664)

* Update redb to 1.4.0 (ordinals#2714)

* Refactor inscriptions paginations (ordinals#2715)

* Display table stats in `ord index info` (ordinals#2711)

* Use redb's recovery callback API (ordinals#2584)

* Allow setting CSP origin (ordinals#2708)

* Remove default file path from `ord index export --tsv` (ordinals#2717)

* Use icons in nav bar (ordinals#2722)

* Add Debian packaging instructions (ordinals#2725)

* Add Homebrew install instructions to readme (ordinals#2726)

* Add sat recursive endpoints with index and pagination (ordinals#2680)

* Only accept sat number in recursive endpoint (ordinals#2732)

* Fix typo in docs/src/inscriptions/metadata.md (ordinals#2731)

* Add docs for metadata recursive endpoint (ordinals#2734)

* Remove `RUNE` from <h1> on /rune (ordinals#2728)

* Add /r/children recursive endpoint (ordinals#2431)

* Add docs and examples for sat recursive endpoint (ordinals#2735)

* Ignore flaky test (ordinals#2742)

* Update docs to include all fields, including content-encoding (ordinals#2740)

* Add docs for child recursive endpoint (ordinals#2743)

* Hide JSON and .btc (ordinals#2744)

* Release 0.12.0 (ordinals#2746)

---------

Co-authored-by: raph <[email protected]>
Co-authored-by: duttydeedz <[email protected]>
Co-authored-by: Casey Rodarmor <[email protected]>
Co-authored-by: liam <[email protected]>
Co-authored-by: Eloc <[email protected]>
Co-authored-by: Julian Eager <[email protected]>
Co-authored-by: ordinally <[email protected]>
Co-authored-by: Christopher Berner <[email protected]>
Co-authored-by: Rijndael <[email protected]>
Co-authored-by: vuittont60 <[email protected]>
thedoublejay pushed a commit to sadoprotocol/ord that referenced this pull request Dec 1, 2023
* Fix lost sats bug (ordinals#2666)

* Add Hindi version of handbook (ordinals#2648)

* Remove Index::index_block_inscription_numbers (ordinals#2667)

* Hide protocol inscriptions (ordinals#2674)

* Don't color links in headers (ordinals#2678)

* Add inscription charms (ordinals#2681)

* Group rune server tests (ordinals#2685)

* Add inscription compression (ordinals#1713)

* Fix media table formatting (ordinals#2686)

* Update schema version for charms (ordinals#2687)

* Fix unbound outpoint server error (ordinals#2479)

* Add binary media type (ordinals#2671)

* Clean up install.sh (ordinals#2669)

* Add /collections Page (ordinals#2561)

* Preview font inscriptions (ordinals#2692)

* Only load used language highlight module in code preview (ordinals#2696)

* Only try to create the database if it wasn't found (ordinals#2703)

* Move postage into batch file (ordinals#2705)

* Add destination field to batch (ordinals#2701)

* Use sequence numbers database keys (ordinals#2664)

* Update redb to 1.4.0 (ordinals#2714)

* Refactor inscriptions paginations (ordinals#2715)

* Display table stats in `ord index info` (ordinals#2711)

* Use redb's recovery callback API (ordinals#2584)

* Allow setting CSP origin (ordinals#2708)

* Remove default file path from `ord index export --tsv` (ordinals#2717)

* Use icons in nav bar (ordinals#2722)

* Add Debian packaging instructions (ordinals#2725)

* Add Homebrew install instructions to readme (ordinals#2726)

* Add sat recursive endpoints with index and pagination (ordinals#2680)

* Only accept sat number in recursive endpoint (ordinals#2732)

* Fix typo in docs/src/inscriptions/metadata.md (ordinals#2731)

* Add docs for metadata recursive endpoint (ordinals#2734)

* Remove `RUNE` from <h1> on /rune (ordinals#2728)

* Add /r/children recursive endpoint (ordinals#2431)

* Add docs and examples for sat recursive endpoint (ordinals#2735)

* Ignore flaky test (ordinals#2742)

* Update docs to include all fields, including content-encoding (ordinals#2740)

* Add docs for child recursive endpoint (ordinals#2743)

* Hide JSON and .btc (ordinals#2744)

* Release 0.12.0 (ordinals#2746)

* Hide all text (ordinals#2753)

* Add batch to preview command (ordinals#2752)

* Add stuttering curse (ordinals#2745)

* Batch inscribe on same sat (ordinals#2749)

* Allow setting the sat to inscribe (ordinals#2765)

* Select further away coins which meet target (ordinals#2724)

* Fix typos (ordinals#2768)

* Add ability to specify sat to batch inscribe (ordinals#2770)

* Add commands to etch and list runes (ordinals#2544)

* Set CSP origin in deploy script (ordinals#2764)

Co-authored-by: raph <[email protected]>

* Add `public` to /content Cache-Control headers (ordinals#2773)

* Release 0.12.1 (ordinals#2776)

* Bless cursed inscriptions after Jubilee height (ordinals#2656)

* Hide /content/<INSCRIPTION_ID> HTML inscriptions (ordinals#2778)

* Release 0.12.2 (ordinals#2780)

* fix(test): error test from version 0.12.2

---------

Co-authored-by: raph <[email protected]>
Co-authored-by: duttydeedz <[email protected]>
Co-authored-by: Casey Rodarmor <[email protected]>
Co-authored-by: liam <[email protected]>
Co-authored-by: Eloc <[email protected]>
Co-authored-by: Julian Eager <[email protected]>
Co-authored-by: ordinally <[email protected]>
Co-authored-by: Christopher Berner <[email protected]>
Co-authored-by: Rijndael <[email protected]>
Co-authored-by: vuittont60 <[email protected]>
Co-authored-by: gmart7t2 <[email protected]>
Co-authored-by: xiaolou86 <[email protected]>
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.

3 participants