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

Experimental Inscription Support #3

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

Experimental Inscription Support #3

wants to merge 6 commits into from

Conversation

JeremyRubin
Copy link

No description provided.

if ret.last() == Some(&Token::Num(0)) {
// Inscription Detected
ret.pop();
if let Some(Ok(Instruction::PushBytes(b"ord"))) = it.next() {

Choose a reason for hiding this comment

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

might be good to use PROTOCOL_ID instead of b"ord" here since you already have it as a const, and in-case you ever want to parse other envelope-using protocols

Copy link
Author

Choose a reason for hiding this comment

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

the reason is more annoying than that. it's because pattern matching on consts is IMO sketchy... to make it work "right" you want to do ord::PROTOCOL_ID (so it isn't accidentally an unchecked ID), and even then, I couldn't get the compiler to accept it because array/slice stuff. If you know how to get it to work LMK.

Choose a reason for hiding this comment

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

yeah, you're right :-/ unless you wanted to do something like

if let Some(Ok(Instruction::PushBytes(bytes))) = it.next() {
    if bytes == PROTOCOL_ID {
        // whatever
    }
}

this approach is better

Copy link
Author

Choose a reason for hiding this comment

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

the compiler is no match for me.

Copy link
Author

Choose a reason for hiding this comment

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

I kinda want to stick a comment in there not to remove the name ord::

let metaprotocol = Tag::Metaprotocol.remove_field(&mut fields);
let parent = Tag::Parent.remove_field(&mut fields);
let pointer = Tag::Pointer.remove_field(&mut fields);

Choose a reason for hiding this comment

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

there's also an unbound tag (66). are you just having it get rolled into unrecognized_even_field?

Copy link
Author

Choose a reason for hiding this comment

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

I coppied this code from the ord repo so i'd do whatever they say! is 66 good?

@@ -115,6 +115,8 @@ pub mod interpreter;
pub mod miniscript;
pub mod policy;
pub mod psbt;
#[allow(missing_docs)]
pub mod ord;

Choose a reason for hiding this comment

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

Would it be useful to you if we exported the inscriptions module in ord and made more types public so you can use it as a crate?

Copy link
Author

Choose a reason for hiding this comment

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

yes, I think it's a good idea. but also not really for sapio unfortunately since we have like the entire rust-bitcoin ecosystem forked with patches.

this patch set though should be upstreamable!

Choose a reason for hiding this comment

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

Would it be useful to you if we exported the inscriptions module in ord and made more types public so you can use it as a crate?

I would love that.

Choose a reason for hiding this comment

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

Have a look if this is useful: ordinals/ord#3042

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