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

implement burn #2766

Closed

Conversation

onchainguy-btc
Copy link
Contributor

PR for #2597

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.

Thanks for the PR! Just doing a quick review for now.

  • There are a lot of whitespace changes, can you reformat to remove them?
  • I'd prefer not to introduce a new struct to represent an OutputScript. Can we just use a ScriptBuf? There is a helper function to convert a script buf to an address, if possible.
  • We should use the helper functions ScriptBuf::new_op_return and Script::is_op_return, to create a new op return and check if an output is an op return.
  • Op returns can contain data pushes, OP_N opcodes, and OP_RESERVED. I think we should have a plan that allows us to display both correctly, and eventually allow users to perform both types of burns. I'm thinking that we should burn with a marker that indicates that this is a human-readable text. So if we're burning text, what winds up on chain is OP_RETURN MARKER <PUSHES>. If the OP_RETURN matches that pattern, we collect the pushes into a string and display them. Otherwise we display the script. For the marker, I'm thinking we should use OP_1NEGATE. OP_1NEGATE is pretty useless in an OP_RETURN, it pushes 0x83 onto the stack, and is pretty much only useful when evaluating a Bitcoin Script, so isn't very useful in an OP_RETURN.
  • Let's add a burn charm, it should be 🔥, natch.
  • Burn OP_RETURNS should always have value 0. We should add a test for that.

@onchainguy-btc
Copy link
Contributor Author

@casey thanks for the feedback. All makes sense. Except the problem with the Script helper is that it only allows to create p2pkh, p2sh or witness programs from script. It will fail with an UnrecognizedScript error if you try it with an OP_RETURN. We could create our own helper but actually then going for the extra struct might be cleaner.

@casey
Copy link
Collaborator

casey commented Dec 1, 2023

@casey thanks for the feedback. All makes sense. Except the problem with the Script helper is that it only allows to create p2pkh, p2sh or witness programs from script. It will fail with an UnrecognizedScript error if you try it with an OP_RETURN. We could create our own helper but actually then going for the extra struct might be cleaner.

I'm not sure I understand where specifically this is an issue. I know we need to convert the script to an address in some places, but if conversion fails, can we substitute the correct value for an op return with unwrap_or_else?

@onchainguy-btc
Copy link
Contributor Author

@casey thanks for the feedback. All makes sense. Except the problem with the Script helper is that it only allows to create p2pkh, p2sh or witness programs from script. It will fail with an UnrecognizedScript error if you try it with an OP_RETURN. We could create our own helper but actually then going for the extra struct might be cleaner.

I'm not sure I understand where specifically this is an issue. I know we need to convert the script to an address in some places, but if conversion fails, can we substitute the correct value for an op return with unwrap_or_else?

lol, you're right. I'm a rust noob sorry 💀 Will get done with the changes!

@casey
Copy link
Collaborator

casey commented Dec 1, 2023

lol, you're right. I'm a rust noob sorry 💀 Will get done with the changes!

No worries, Rust is hard!

@onchainguy-btc
Copy link
Contributor Author

@casey was just about to complete the changes, one thing I wanted to discuss is setting OP_RETURN value to 0 (maybe I don't get what you mean). If we set the value to 0, we need to store some info on the inscription (in ord) that it has been burned because the sats still exist. So the sats would be freed which is nice. However, marketplaces would also need to fetch the info to avoid people buying burned inscriptions in a collection. Now somebody might list an inscription on a marketplace but then submit a burn shortly before the buyer gets it. I mean yes, the buyer gets it but it would have the burned property. Instead if we just burn the sats, the inscription can't be traded at all anymore. The sats might be "wasted" but a little deflation for BTC might be ok, people won't really care to much in most scenarios. We could also have different burn modes if we would like to support both.

@casey
Copy link
Collaborator

casey commented Dec 5, 2023

@onchainguy-eth Whoops, I'm an idiot, of course the OP_RETURN has to have a nonzero value in order to burn the sat 😅

@onchainguy-btc onchainguy-btc force-pushed the burn-inscriptions branch 2 times, most recently from 4c69287 to d22e78c Compare December 9, 2023 20:15
@onchainguy-btc
Copy link
Contributor Author

@casey just pushed an update with all the requested changes. Some stuff wasn't sure if best way to do it like e.g. assigning the charm.

@onchainguy-btc
Copy link
Contributor Author

Just rebased the rune changes

@casey casey linked an issue Feb 12, 2024 that may be closed by this pull request
@onchainguy-btc onchainguy-btc mentioned this pull request Apr 1, 2024
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.

Allow burning inscriptions
2 participants