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

sat recursive endpoint #2452

Closed
wants to merge 20 commits into from

Conversation

elocremarc
Copy link
Contributor

@elocremarc elocremarc commented Sep 15, 2023

See discussions in #2277 and #2200

Added 2 endpoints
̶̶/̶c̶o̶n̶t̶e̶n̶t̶/̶s̶a̶t̶/̶:̶s̶a̶t̶/̶:̶p̶a̶g̶e̶_̶i̶n̶d̶e̶x̶ ̶-̶>̶ ̶C̶o̶n̶t̶e̶n̶t̶̶
/sat/:sat/:number -> {id: <InscriptionId>}
Update: We removed route 1 are only returning json.

Second route negative number is most recent inscription. I made it Json so its extendable if we ever want to add other things in the future like block, rarity, length of inscritions index, date we have the option to not rug legacy inscriptions using this endpoint.

Both are beneficial routes first can be use directly in recursion. Second can be used to link to those inscriptions.
Another benefit of the second route is if artist wishes to stop rending their inscription if its been reinscribed they check.
/sat/:sat/1 === /sat/:sat/-1 if this is false then it means there are > 1 inscriptions and they could stop rendering.

Question:
I can't get the CSP headers working because its a nested dynamic route should this be refactored to use query paramaters instead?

@lifofifoX
Copy link
Collaborator

Instead of a separate /sat/:sat/:number recursive endpoint, why not expose the existing /sat/:sat endpoint via recursion? That endpoint is already returning JSON with all the relevant information about the sat.

@elocremarc
Copy link
Contributor Author

elocremarc commented Sep 21, 2023

Instead of a separate /sat/:sat/:number recursive endpoint, why not expose the existing /sat/:sat endpoint via recursion? That endpoint is already returning JSON with all the relevant information about the sat.

@devords We don't want to expose all that as an API this would make it so people wouldn't run a full sat index and would instead use ordinals.com as an API. /sat/:number currently isn't returning json on ordinals.com. This solution leaves the possibility of adding some of that information at a later date. Also we don't want to return the entire list of inscriptions we only want to return one at a time. That is why this has the /:number

I made it Json so its extendable if we ever want to add other things in the future like block, rarity, length of inscriptions index, date we have the option to not rug legacy inscriptions using this endpoint.

@lifofifoX
Copy link
Collaborator

@elocremarc Ah! That makes sense 👍 I guess my main concerns is the endpoint URLs being too much alike. It probably makes sense to clearly separate recursive API endpoints from explorer API.

@elocremarc
Copy link
Contributor Author

elocremarc commented Sep 23, 2023

@raphjaph What are you thoughts on this and my questions about the CSP headers? I want to find a solution so I can fix the other PR as well to follow this format. #2431 (comment)

Edit: Resolved with /-/ in CSP headers #2476

@elocremarc elocremarc force-pushed the recursive-endpoint-sat branch from 1abd668 to 15e9ba7 Compare October 2, 2023 02:18
@elocremarc elocremarc marked this pull request as ready for review October 2, 2023 20:18
@elocremarc
Copy link
Contributor Author

@raphjaph this ones ready for review. Changed to the /-/ for recursive endpoints.

@lifofifoX
Copy link
Collaborator

Any thoughts on using limit and offset query params for paginating? To begin with, limit can be set to a fixed number like 1000.

Page 1

/s/:sat

{
  "limit": 1000,
  "offset": 0,
  "count": 1211,
  "inscriptions": [
    "inscription_0",
    "inscription_1",
    ...
    "inscription_999"
  ]
}

Page 2

/s/:sat?offset=1000

{
  "limit": 1000,
  "offset": 1000,
  "count": 1211,
  "inscriptions": [
    "inscription_1000",
    "inscription_1001",
    ...
    "inscription_1210"
  ]
}

Similar pattern could be used wherever we need to return multiple inscription IDs. With this approach, inscriptions can avoid making N requests to fetch that many inscription IDs.

@zmeyer44
Copy link
Contributor

zmeyer44 commented Oct 4, 2023

/sat/:sat/1 === /sat/:sat/-1 if this is false then it means there are > 1 inscriptions and they could stop rendering.

Shouldn't we start with /sat/:sat/0 being the first inscription on the sat and /sat/:sat/1 being the second? This would be more consistent with common array indexing function such as .at(0) in Javascript

@elocremarc
Copy link
Contributor Author

elocremarc commented Oct 5, 2023

Any thoughts on using limit and offset query params for paginating? To begin with, limit can be set to a fixed number like 1000.

1000 probably too many I imagine.

You would also want reverse if we return an array because then you would have to check length calculate the end to get the most recent inscription. Where you could get the most recent with just -1

The way this is setup now is it returns just one at a time negative number is basically the reverse call. This saves on bytes because the request for getting the most recent inscription is very compact. Opposed to a reverse with either iterating through the pagination and popping off the end if we have no reverse. Or adding a reverse where you need an extra param/path then still need to pop the array. This would require at least 2 calls and more code.

See Post on #2277

/sat/N # first inscription on sat N
/sat/N/X # Xth inscription on sat N, oldest first
/sat/N/-X # Xth inscription on sat N, newest first

@devords I saw you commented in that issue and I agree we don't need the first endpoint because /1 would be the first.

Shouldn't we start with /sat/:sat/0 being the first inscription on the sat and /sat/:sat/1 being the second? This would be more consistent with common array indexing function such as .at(0) in Javascript

I would agree if we are returning an array but were just returning the first and the last count based on negative number. Therefore under that convention would we do -0 and 0? See discussion in 2277 and here #2200

I do think it could be beneficial to return length in the JSON so you have insight into how to how many inscriptions a sat has. However you could just stop iterating when /-1 === /N to get the count.

I can see how an array would also be useful however I did this PR to address the 2 issues already discussed. Its pretty elegant and compact. Array would def be better for higher numbers of inscriptions but it would add a lot of extra code.

@@ -885,6 +909,36 @@ impl Server {
Some((headers, inscription.into_body()?))
}

async fn content_sat(
Copy link
Contributor Author

@elocremarc elocremarc Oct 23, 2023

Choose a reason for hiding this comment

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

@raphjaph this question is outside the route name. Do we only want to return the id from this PR? Should I take out this content endpoint I think you said that somewhere in the long thread?
/content/sat/:sat/:page_index -> Content

@null-ish
Copy link

LFG

@elocremarc
Copy link
Contributor Author

elocremarc commented Nov 1, 2023

@raphjaph Should we add 2 endpoints one with pagination and one for just most recent or first or N?
Both are useful for different use cases.
Pagination is good for making galerys while the /-1 is good for people who just want to have updated logic of most recent inscirption. We could use -1 and 1 as the reverse and add a page to get a list of ids.

/r/sat/:satnumber/:index example /r/sat/42069/-1 -> {id: "xxxx"}
/r/sat/:satnumber/:reverse/:index example /r/sat/42069/-1/2 -> {ids: [{id: "xxxx"}...] in reverse order

The most common use case people want this endpoint for is for the first endpoint to update inscriptions. If we use the second one then it will take a lot more JS in inscriptions to get the result of endpoint 1. Almost all of the discusion including the 2 linked issues in the OP were all around the -1 indexing. See my long comment above about the pros and cons of each. Its quite frustrating doing PRs off the consensus of the discursion and then it gets changed at the whims of the maintainers without any discussion on making these changes. For instance in the directory /r/ was never discused and was just merged in.

@rot13maxi
Copy link
Contributor

playing with this branch, I agree it would be nice to 0-index inscriptions instead of 1-indexing them.

@rot13maxi
Copy link
Contributor

This endpoint requires a sat-index. I don't usually run a sat-index because it takes a looooooong time to (re)build. Do we (anyone :-) ) have a sense of how many marketplaces/wallets/explorers run with the full sat index? This seems like a really useful endpoint, but I'm trying to think through how much artifacers can depend on support for it and what fallback behavior we can build if its unsupported in a particular viewer... Any thoughts?

@cbspears
Copy link
Contributor

cbspears commented Nov 2, 2023

Do we (anyone :-) ) have a sense of how many marketplaces/wallets/explorers run with the full sat index? This seems like a really useful endpoint, but I'm trying to think through how much artifacers can depend on support for it and what fallback behavior we can build if its unsupported in a particular viewer... Any thoughts?

There are only a handful of explorers running their own full sat index, with most of the ecosystem relying on those other indices. I want to be tactful here but here's my assumptions atm, would love anyone's clarification: The only ones I can confidently say are their own are Greg's & Ordinals.com. Hiro/Leather appears to have initially synced from Gregs (pretty much everyone just syncs from Gregs tbh). Then most of the eco uses Hiro/leather so by extension using Greg's. OrdinalHub does everything internal and hasn't publicized a full sat index at this time.

Are there any others I am missing? I'm sure I'll come back to update or edit this when I realize I missed something.

@elocremarc
Copy link
Contributor Author

This endpoint requires a sat-index. I don't usually run a sat-index because it takes a looooooong time to (re)build. Do we (anyone :-) ) have a sense of how many marketplaces/wallets/explorers run with the full sat index? This seems like a really useful endpoint, but I'm trying to think through how much artifacers can depend on support for it and what fallback behavior we can build if its unsupported in a particular viewer... Any thoughts?

For testing you can run regtest with satindex. Yeah whenever we get to documenting this we should stress that people should use try catch blocks so that normal functionality can still remain and not blow up your inscription if the endpoint is never found on explorers that don't have the sat index.

@elocremarc
Copy link
Contributor Author

playing with this branch, I agree it would be nice to 0-index inscriptions instead of 1-indexing them.

Thoughts on this?

I would agree if we are returning an array but were just returning the first and the last count based on negative number. Therefore under that convention would we do -0 and 0? See discussion in #2277 and here #2200

I still think that having 2 options array with 0 index pagination and a short request to get the most recent inscription or first with -1 and 1 respectively would benefit different use cases and make inscription code more concise for people who just want the most recent content which seems like a vast majority of people I talk to about this endpoint.

@rot13maxi
Copy link
Contributor

when I played with this, I was expecting 0 to be the first inscription and -1 to be the last. I think that's just what I assume when I see an interface to get the nth of something.

I completely agree that the common case is probably going to be "most recent". But there will probably be some usecases (gallery, list of markdown articles, game selector, etc) where you'll want to iterate through all the inscriptions, throw them in an array, and either have the user or some other part of your program pick one to load/view/whatever. In those cases, I think 1-indexing is inviting off-by-one errors.

@elocremarc
Copy link
Contributor Author

elocremarc commented Nov 3, 2023

Hmm this might be the best way to do it actually as seen by the inscription.

# Descending 
fetch(`${window.location.origin}/r/sat/42069/d`)
  .then(response => response.json())
  .then(data => {
    const lastId = data.ids[0].id;
    console.log(lastId);  // Outputs: "Most Recent Inscription page 1"
    console.log(data.count); //Outputs: "Total Count of Inscriptions"
    const isReinscribed = data.count > 1 ? true : false;
    console.log(isReinscribed) //outputs: "True if reinscribed False if not"
  })
  
# Ascending
  fetch(`${window.location.origin}/r/sat/42069/a/2`)
  .then(response => response.json())
  .then(data => {
    const hundredAndOne = data.ids[0].id;
    console.log(hundredAndOne);  // Outputs: "101th inscription on page 2"
  })
 fetch(`${window.location.origin}/r/sat/42069/a`)
  .then(response => response.json())
  .then(data => {
    const first = data.ids[0].id;
    const hundredth = data.ids[99].id;
    console.log(first);  // Outputs: "1st inscription on page 1"
    console.log(hundredth): // Outputs: "100th inscription on page 1"
  })

Downside is we throw away 99 of the Ids in Descending requests because we don't care about them here in this example.

@raphjaph
Copy link
Collaborator

raphjaph commented Nov 6, 2023

I think we want to have to endpoint, one that returns a single id and another that returns a paginated list of inscriptions ids. Both should definitely be 0-indexed. This is my thinking at the moment:

Let's NOT return sat info since it's static:
/r/sat/:sat_number -> first OR information on sat (like rarity, etc.)?`

Returning single id:
/r/sat/:sat_number/0 -> first
/r/sat/:sat_number/-1 -> most recent
/r/sat/:sat_number/-100 -> 100th most recent

"c3a05e88abd51708954ce562389478028567184e32fb96ab8d07b672989155b8i0", 

OR

{  
  "id": "c3a05e88abd51708954ce562389478028567184e32fb96ab8d07b672989155b8i0", 
}

The last would make it easier to add fields later.

Returning paginated JSON with array of 100 inscriptions per page:
/r/inscriptions/sat/:number -> first set of 100 inscription ids
/r/inscription/sat/:number/0 -> first set of 100 inscription ids
/r/inscriptions/sat/:number/3-> inscriptions 300-400 on a sat

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

We could add a total count of inscription ids in the JSON responses but this would require us to calculate the length of the table for every request.

@raphjaph
Copy link
Collaborator

raphjaph commented Nov 6, 2023

This endpoint requires a sat-index. I don't usually run a sat-index because it takes a looooooong time to (re)build. Do we (anyone :-) ) have a sense of how many marketplaces/wallets/explorers run with the full sat index? This seems like a really useful endpoint, but I'm trying to think through how much artifacers can depend on support for it and what fallback behavior we can build if its unsupported in a particular viewer... Any thoughts?

Unfortunately I don't think there is any good fallback behaviour that explorers can provide if they don't have the sat index other than returning a 404 Not Found. We should do this in ord as well. At the moment we return an ugly 500 Internal Server Error. The fallback would have to happen on the inscription content level, so just some try catch stuff in javascript. Haven't thought about this deeply though so maybe there is a more elegant way.

@raphjaph
Copy link
Collaborator

raphjaph commented Nov 6, 2023

@elocremarc start by implementing the first endpoint, so you don't have to worry about pagination

@elocremarc
Copy link
Contributor Author

start by implementing the first endpoint, so you don't have to worry about pagination

@raphjaph removed the sat info so the first endpoint should be gtg lgtm!

Copy link
Collaborator

@raphjaph raphjaph left a comment

Choose a reason for hiding this comment

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

Probably have to wait for #2664 to make this efficient. Hopefully soon!

@@ -229,6 +229,7 @@ impl Server {
.route("/r/blockheight", get(Self::block_height))
.route("/r/blocktime", get(Self::block_time))
.route("/r/metadata/:inscription_id", get(Self::metadata))
.route("/r/sat/:sat/:page_index", get(Self::sat_from_n))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.route("/r/sat/:sat/:page_index", get(Self::sat_from_n))
.route("/r/sat/:sat_number/:page_index", get(Self::sat_inscriptions))

async fn sat_from_n(
Extension(index): Extension<Arc<Index>>,
Path((DeserializeFromStr(sat), DeserializeFromStr(n))): Path<(
DeserializeFromStr<Sat>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shoudl not accept a Sat but only a sat number so u64

Extension(index): Extension<Arc<Index>>,
Path((DeserializeFromStr(sat), DeserializeFromStr(n))): Path<(
DeserializeFromStr<Sat>,
DeserializeFromStr<i64>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be a plain i64, it does not need to be deserialized from a string

DeserializeFromStr<i64>,
)>,
) -> ServerResult<Json<SatId>> {
let mut inscriptions = index.get_inscription_ids_by_sat(sat)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is very inefficient at the moment because it loads all inscriptions on a sat and goes to multiple tables to order the multimap result. Prob have to wait for #2664 to make this efficient

Comment on lines 504 to 514
let n_usize = if n < 0 {
inscriptions.reverse();
usize::try_from((-n - 1).abs())
} else {
usize::try_from(n)
}
.map_err(|_| ServerError::BadRequest("Invalid request".to_string()))?;
let inscription_id = inscriptions
.get(n_usize)
.ok_or_else(|| ServerError::NotFound("Inscription not found".to_string()))?;
Ok(Json(SatId {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The above would also clean up all of this logic

);

server.assert_response_regex(
"/r/sat/5000000000/-1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shoudl think about what happens if you pass in an index that is out of bounds. Should we return a 404 or a json with a descriptive error?

@@ -26,6 +26,11 @@ pub struct SatJson {
pub inscriptions: Vec<InscriptionId>,
}

#[derive(Debug, Serialize, Deserialize)]
pub struct SatId {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub struct SatId {
pub struct SatInscription {

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.

7 participants