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

Allow multiple delegates. #3301

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

Conversation

arik-so
Copy link
Contributor

@arik-so arik-so commented Mar 17, 2024

This PR modifies server behavior to return the content of the first delegate it finds, absent which it returns the inscription's own content.

#4114

@arik-so arik-so marked this pull request as draft March 17, 2024 04:17
let inscription_override = inscription
.delegates()
.iter()
.find_map(|delegate| index.get_inscription_by_id(*delegate).unwrap_or(None));
Copy link
Contributor Author

@arik-so arik-so Mar 20, 2024

Choose a reason for hiding this comment

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

I'm really worried about this being a DoS vector. It would be fairly trivial to inscribe something with a couple thousand non-existent delegates. Any thoughts on whether it's a) a non-issue, b) makes the multi-delegate approach altogether unviable, or c) there are some mitigating strategies? @raphjaph

Copy link

@cryptocj cryptocj Mar 22, 2024

Choose a reason for hiding this comment

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

Can we add a limit number to avoid the Dos concern?
And extract a function to remove the repeat code below.

fn find_inscription<'a>(delegates: &'a [usize], index: &'a Index, max_length: usize) -> Option<&'a Inscription> {
    delegates
        .iter()
        .take(max_length)
        .find_map(|delegate| index.get_inscription_by_id(*delegate).unwrap_or(None))
}

// Usage
let inscription_override = find_inscription(inscription.delegates(), &index, max_length);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, the question is just what the limit should be. That's why I think @casey or @raphjaph should weigh in.

@arik-so
Copy link
Contributor Author

arik-so commented Mar 20, 2024

This PR needs a test that, on the one hand, would allow the creation a custom inscription witness with multiple delegates, while on the other hand allowing to query the /content API endpoint.

The difficulty lies in the fact that Inscription::to_witness and InscriptionId::value are only available in ord::src, whereas TestServer is only available within ord::test. It seems like an inelegant merging of responsibilities to publicly expose the inscription-related methods, or to copy TestServer into ord::src, so if this is worth pursuing, I'm open to creative suggestions from the maintainers.

@arik-so arik-so marked this pull request as ready for review November 24, 2024 06:34
@casey
Copy link
Collaborator

casey commented Nov 26, 2024

I think this is fine, and I'm not to worried about the DoS potential. If it becomes a problem because someone created an inscription with a million delegates, we could just introduce a cap on the number of delegates that ordinals.com will respect. We could also be proactive, and limit it to, say, ten delegates, and just ignore any after that.

We've talked about recursive delegates before, see #2922. And I guess this probably works fine with that.

However, I'm not totally sure I understand the motivation for this feature. Can you open an issue to discuss it and tag me? I want to make sure that I understand what problem this solves or benefit it adds, and that whatever we do is the best possible solution.

@raphjaph
Copy link
Collaborator

For reference #4114

%% if self.inscription.content_length().is_some() || self.inscription.delegate().is_some() {
%% if let Some(delegate) = self.inscription.delegate() {
<dt>delegate</dt>
%% if self.inscription.content_length().is_some() || !self.inscription.delegates().is_empty() {
Copy link
Collaborator

@raphjaph raphjaph Dec 11, 2024

Choose a reason for hiding this comment

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

How does this interact with inscriptions that have no content but do have delegates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self.inscription.content_length().is_some() appears entirely superfluous, as the inner check is just if we have delegates, and the outer check is if we either have some content or delegates.

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.

Seems mostly reasonable to me! We should always handle errors instead of doing unwrap_or, or some other suppression.

I think we should impose a limit of 100 delegates, and any delegates after are just ignored. I can't imagine someone needing more, and it's more painful to add a restriction later than to relax a restriction.

let effective_mime_type = inscription
.delegates()
.iter()
.find_map(|delegate| self.get_inscription_by_id(*delegate).unwrap_or(None))
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should handle the error here instead of using unwrap_or.

let inscription_override = inscription
.delegates()
.iter()
.find_map(|delegate| index.get_inscription_by_id(*delegate).unwrap_or(None));
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should handle the error here instead of using unwrap_or. All errors must be handled >:]

let inscription_override = inscription
.delegates()
.iter()
.find_map(|delegate| index.get_inscription_by_id(*delegate).unwrap_or(None));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Handle errors.

@@ -118,7 +118,7 @@ pub struct InscriptionRecursive {
pub charms: Vec<Charm>,
pub content_type: Option<String>,
pub content_length: Option<usize>,
pub delegate: Option<InscriptionId>,
pub delegates: Vec<InscriptionId>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We cannot change the return type since it's a recursive endpoint. We'll have to return the effective delegate here instead in an Option<InscriptionId>.

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.

4 participants