-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
base: master
Are you sure you want to change the base?
Conversation
let inscription_override = inscription | ||
.delegates() | ||
.iter() | ||
.find_map(|delegate| index.get_inscription_by_id(*delegate).unwrap_or(None)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 The difficulty lies in the fact that |
5d399ca
to
2929b5d
Compare
2929b5d
to
51b9200
Compare
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. |
This PR modifies server behavior to return the content of the first delegate it finds, absent which it returns the inscription's own content.