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

Enable collections/provenance through parent inscriptions method #1869

Closed
wants to merge 34 commits into from

Conversation

raphjaph
Copy link
Collaborator

@raphjaph raphjaph commented Mar 4, 2023

Implementation work on #783, look there for more information.

src/inscription.rs Outdated Show resolved Hide resolved
@veryordinally
Copy link
Collaborator

ord wallet inscribe is still broken with this PR:

error: Failed to send reveal transaction
because: JSON-RPC error: RPC error response: RpcError { code: -26, message: "non-mandatory-script-verify-flag (Invalid Schnorr signature)", data: None }

Ordinally and others added 5 commits March 8, 2023 20:25
```
error: Failed to send reveal transaction
because: JSON-RPC error: RPC error response: RpcError { code: -26, message: "non-mandatory-script-verify-flag (Invalid Schnorr signature)", data: None }
```
```
thread 'main' panicked at 'signature hash should compute: PrevoutKind', src/subcommand/wallet/inscribe.rs:360:8
```

#[derive(Debug, PartialEq, Clone)]
pub(crate) struct Inscription {
body: Option<Vec<u8>>,
parent: Option<InscriptionId>,
Copy link

Choose a reason for hiding this comment

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

This is a more general point, but I was curious if there is a technical reason behind having only a single parent (vs. a list of 1 or more)?

I understand that in most cases people may want just one but there are also lots of usecases where you may want more than one. A basic example is a mashup (I take one inscription and another inscription and create a new mashup art). A more advanced example is "crafting", where I may take some set of inscriptions as parents, create a child, and burn the parents (all in one tx).

reveal TX for inscriptions containing a parent.
@veryordinally
Copy link
Collaborator

After some deep-diving on decoding the partially signed and fully signed reveal TXs when inscribing with a parent, I found that sign_raw_transaction_with_wallet apparently removes our inscription data from the witness data. Need to dig deeper to find out why.

@raphjaph @casey it would be great to catch this in tests, but I wasn't sure how to best fit this kind of test into the testing framework. Happy to discuss.

@@ -278,7 +382,7 @@ impl Inscribe {
);

let witness = sighash_cache
.witness_mut(0)
.witness_mut(commit_input_offset)
.expect("getting mutable witness reference should work");
witness.push(signature.as_ref());

Choose a reason for hiding this comment

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

When inscribing with a parent, do we need to encode the signature with the proper SigHashType before adding to the witness?

let encoded_sig = SchnorrSig { sig: signature, hash_ty: SchnorrSighashType::AllPlusAnyoneCanPay };
witness.push(encoded_sig.serialize());

None => None,
Some(bytes) => {
if bytes.len() != 36 {
return Err(InscriptionError::InvalidInscription)

Choose a reason for hiding this comment

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

Might just ignore and return None here instead of throwing Error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, this part unfinished, but you're right, we should gracefully ignore malformed inscriptions.

src/inscription.rs Outdated Show resolved Hide resolved
Comment on lines +105 to +142
let array = [
head_array[0],
head_array[1],
head_array[2],
head_array[3],
head_array[4],
head_array[5],
head_array[6],
head_array[7],
head_array[8],
head_array[9],
head_array[10],
head_array[11],
head_array[12],
head_array[13],
head_array[14],
head_array[15],
tail_array[0],
tail_array[1],
tail_array[2],
tail_array[3],
tail_array[4],
tail_array[5],
tail_array[6],
tail_array[7],
tail_array[8],
tail_array[9],
tail_array[10],
tail_array[11],
tail_array[12],
tail_array[13],
tail_array[14],
tail_array[15],
index_array[0],
index_array[1],
index_array[2],
index_array[3],
];
Copy link
Contributor

Choose a reason for hiding this comment

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

A suggestion to improve code readability:

Suggested change
let array = [
head_array[0],
head_array[1],
head_array[2],
head_array[3],
head_array[4],
head_array[5],
head_array[6],
head_array[7],
head_array[8],
head_array[9],
head_array[10],
head_array[11],
head_array[12],
head_array[13],
head_array[14],
head_array[15],
tail_array[0],
tail_array[1],
tail_array[2],
tail_array[3],
tail_array[4],
tail_array[5],
tail_array[6],
tail_array[7],
tail_array[8],
tail_array[9],
tail_array[10],
tail_array[11],
tail_array[12],
tail_array[13],
tail_array[14],
tail_array[15],
index_array[0],
index_array[1],
index_array[2],
index_array[3],
];
let array = vec![
&head_array[0..=15],
&tail_array[0..=15],
&index_array[0..=3]
].concat().try_into().unwrap();

@ericatallah
Copy link

I've been having trouble getting a child inscription to render properly in the explorer. Looks like a few inscription indexes were hard coded to 0. Not sure if there are more though. Here is a minor fix, hope it helps:
raphjaph#3

@veryordinally
Copy link
Collaborator

Making progress. For anyone watching, this code does NOT meet our usual quality criteria yet, by far - lots of tests missing and some places are just held together by string. Anyone tempted to try this, this code may work, but be aware that using this on live wallets may lead to loss of inscriptions or other unpredictable outcomes.

@@ -43,6 +43,7 @@ struct Inscribe {
inscription: String,
reveal: Txid,
fees: u64,
parent: Option<String>,
Copy link

Choose a reason for hiding this comment

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

Suggested change
parent: Option<String>,
#[allow(dead_code)]
parent: Option<String>,

Copy link

Choose a reason for hiding this comment

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

This is just so that the IDE doesn't show warnings that inscribe.parent isn't being used.

@raphjaph
Copy link
Collaborator Author

Closing because #1963 is the most up to date

@raphjaph raphjaph closed this Apr 13, 2023
@raphjaph raphjaph deleted the collections branch January 31, 2024 18:52
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.

6 participants