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

Feature add indexed recursive inscriptions endpoint #3936

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

twosatsmaxi
Copy link
Contributor

@twosatsmaxi twosatsmaxi commented Sep 5, 2024

Fixes #3923

  • Added tests
  • Lint

@twosatsmaxi twosatsmaxi changed the title Feature add indexed recursive inscriptions Feature add indexed recursive inscriptions endpoint Sep 5, 2024
@twosatsmaxi
Copy link
Contributor Author

@casey @raphjaph can someone review it?

@@ -232,6 +232,10 @@ impl Server {
"/r/children/:inscription_id/inscriptions",
get(Self::child_inscriptions_recursive),
)
.route(
"/r/children/:inscription_id/inscriptions/at/:index",
Copy link
Contributor

@elocremarc elocremarc Sep 8, 2024

Choose a reason for hiding this comment

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

We dont need /inscriptions/ in the route it is too verbose and is plural. We are only returning a single Id here.
We should follow the format of the first child endpoint not the second one. I never got around to doing this feature on the original child endpoint I was going to but ran out of time.

-        "/r/children/:inscription_id/inscriptions/at/:index",
+        "/r/children/:inscription_id/at/:index",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good.

));
assert_eq!(child_json.id, latest_child_inscription_id);
}

Copy link
Contributor

@elocremarc elocremarc Sep 8, 2024

Choose a reason for hiding this comment

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

Need test for /at/0 which would be the first child inscription. As well as /at/1 which is the second child. This checks the off by one error we had in the original sat endpoint PR see discussion about that here

Look at the sat endpoint for reference of /at/:index tests:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do.

@elocremarc
Copy link
Contributor

elocremarc commented Sep 8, 2024

Upon further review I don't think there is any benefit of limiting ourself to using the Struct ChildInscriptionRecursive this is from the newer child endpoint which is a more optmized child endpoint that uses less onchain data. This struct was made for quick pagination at the cost of data for speed. Considering we are only returning the info of a single inscription we can afford to return everything about that inscription and use the InscriptionRecursive struct from /r/inscription

If we used InscriptionRecursive we would gain
content_type
content_length
delegate
value
address

#3771 (comment)

@elocremarc
Copy link
Contributor

Upon further review I don't think there is any benefit of limiting ourself to using the Struct ChildInscriptionRecursive

On second thought I think we should only return the ID.
@casey @raphjaph do you agree?
This is redundant data from /r/inscription. Its useful on /r/children/:inscription_id/inscriptions because it returns 100 inscriptions with data. However this is only returning info about one inscription.

@twosatsmaxi
Copy link
Contributor Author

@raphjaph can you have a look once?

@twosatsmaxi
Copy link
Contributor Author

@casey @raphjaph: still waiting for the review.

@wagedu
Copy link

wagedu commented Oct 1, 2024

Upon further review I don't think there is any benefit of limiting ourself to using the Struct ChildInscriptionRecursive

On second thought I think we should only return the ID. @casey @raphjaph do you agree? This is redundant data from /r/inscription. Its useful on /r/children/:inscription_id/inscriptions because it returns 100 inscriptions with data. However this is only returning info about one inscription.

I'm a bit confused with the lack of hype for address info in general. Considering it's the 1st thing listed right after the ID, on any inscription page...
I'd vote for the whole data in the 100 children.
My current case as example: 4 inscriptions with 100 children each. But instead of 4 queries, it necessitates 400 queries, to compare addresses.

Copy link

@Deviltep Deviltep left a comment

Choose a reason for hiding this comment

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

@twosatsmaxi
Copy link
Contributor Author

@raphjaph @casey can you have a look once and also what do you think about comments by @elocremarc? would like your inputs too before going ahead make those changes, makes sense too me.
The PR has been open since more than a month, would appreicate your reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

Recursion: Get latest child from parent inscription
4 participants