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

Include old_owner for transfers in StreamEvent #9

Merged
merged 3 commits into from
Jun 30, 2023
Merged

Conversation

mi-yu
Copy link

@mi-yu mi-yu commented Jun 29, 2023

  • Fetch old owner of transferred inscription using old_satpoint
  • makes 1 extra RPC call per transfer, but can save work on the consumer side

@@ -601,6 +605,17 @@ mod stream {
)
.unwrap_or(Address::p2sh(&Script::default(), StreamEvent::get_network()).unwrap()),
),
old_owner: old_satpoint.map(|old| {
Address::from_script(
&tx
Copy link
Member

Choose a reason for hiding this comment

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

this tx was not the old_satpoint tx, it should be the previous one

@mi-yu mi-yu marked this pull request as ready for review June 30, 2023 00:17
@@ -682,6 +684,21 @@ mod stream {
.unwrap_or_else(|_| panic!("Inscription should exist: {}", self.inscription_id))
{
self.enrich_content(inscription);
let old_owner: Option<Address> = index
Copy link
Member

Choose a reason for hiding this comment

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

the &TxOut::default() and the None branch are not consistent of the return I think

@@ -584,7 +585,7 @@ mod stream {
block_hash: BlockHash,
) -> Self {
StreamEvent {
version: "4.0.0".to_owned(), // should match the ord-kafka docker image version
version: "4.0.2".to_owned(), // should match the ord-kafka docker image version
Copy link
Member

Choose a reason for hiding this comment

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

oh, should be 4.0.0, the consumer side not smart enough yet

@mi-yu mi-yu merged commit 30af4d9 into 4.x Jun 30, 2023
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.

2 participants