Index Runes explicitly spent to op_return #4293
Conversation
|
In order to fix the tests you will have to filter out all outputs in the #[track_caller]
pub(crate) fn assert_runes(
&self,
mut runes: impl AsMut<[(RuneId, RuneEntry)]>,
mut balances: impl AsMut<[(OutPoint, Vec<(RuneId, u128)>)]>,
) {
let runes = runes.as_mut();
runes.sort_by_key(|(id, _)| *id);
let balances = balances.as_mut();
balances.sort_by_key(|(outpoint, _)| *outpoint);
for (_, balances) in balances.iter_mut() {
balances.sort_by_key(|(id, _)| *id);
}
pretty_assert_eq!(runes, self.index.runes().unwrap());
pretty_assert_eq!(balances, self.index.get_rune_balances().unwrap());
let mut outstanding: HashMap<RuneId, u128> = HashMap::new();
for (_, balances) in balances {
for (id, balance) in balances {
*outstanding.entry(*id).or_default() += *balance;
}
}
for (id, entry) in runes {
pretty_assert_eq!(
outstanding.get(id).copied().unwrap_or_default(),
entry.supply() - entry.burned
);
}
} |
|
@raphjaph Added the check. Ended up being a more involved change, but working well. |
lifofifoX
force-pushed
the
index-rune-burns
branch
from
f0bf303 to
e458d0b
Compare
| if !tx.output[vout].script_pubkey.is_op_return() { | ||
| if let Some(sender) = self.event_sender { | ||
| sender.blocking_send(Event::RuneTransferred { | ||
| outpoint, | ||
| block_height: self.height, | ||
| txid, | ||
| rune_id: id, | ||
| amount: balance.0, | ||
| })?; | ||
| } |
There was a problem hiding this comment.
I don't really consume this event_sender so I'm unsure if people would want sending to an OP_RETURN to be an event.
There was a problem hiding this comment.
Yeah, same. I thought it'd be a good idea to keep existing behavior there. But could argue both ways.
There was a problem hiding this comment.
I'm leaning towards yes, I'll let @casey chime in if he sees this
|
Apologies for letting this sit so long. You were on the right track but were just missing a call to make tests simpler. I've made the changes, please have a look and let me know if this looks good @lifofifoX |
|
@raphjaph Changes look good to me! |
There was a problem hiding this comment.
Always panic on unexpected errors in tests, or when we're looking something up in the index that we expect to be there. This kind of defensive programming can catch a lot of bugs.
We should definitely not change the behavior of the event sender. This is a conceptually separate change, even though it's related to OP_RETURNs. If the client is relying on getting this information, than it could be a tricky bug if they don't.
| if self | ||
| .index | ||
| .get_output_script(outpoint) | ||
| .ok() |
There was a problem hiding this comment.
We should probably unwrap and panic here.
| for (id, entry) in runes { | ||
| pretty_assert_eq!( | ||
| outstanding.get(id).copied().unwrap_or_default(), | ||
| outstanding.get(id).copied().unwrap_or_default() - burned_balances, |
There was a problem hiding this comment.
Add assert above which asserts that burned_balances is correct, similar to the above asserts.
| balances.sort_by_key(|(outpoint, _)| *outpoint); | ||
|
|
||
| for (_, balances) in balances.iter_mut() { | ||
| let mut burned_balances = 0; |
There was a problem hiding this comment.
I would just call this burned. It's not really "balances" because it's just a number. Balances so named because it's a map.
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub fn get_output_script(&self, output: &OutPoint) -> Result<ScriptBuf> { |
There was a problem hiding this comment.
Should this return an error? Test functions should just panic and not return errors, since an error can be ignored and a panic cannot.
| .index | ||
| .get_output_script(outpoint) | ||
| .ok() | ||
| .is_some_and(|script| script.is_op_return()) |
There was a problem hiding this comment.
This should also unwrap. If we expect the output script to be in the index, it is a fatal error for it not to be, and we want to panic so we notice.
| rune_id: id, | ||
| amount: balance.0, | ||
| })?; | ||
| if !tx.output[vout].script_pubkey.is_op_return() { |
There was a problem hiding this comment.
This reduces the amount of information a listener gets. This is a separate change and should be done in a separate PR.
| let outpoint_to_utxo_entry = rtx.open_table(OUTPOINT_TO_UTXO_ENTRY)?; | ||
| let entry = outpoint_to_utxo_entry | ||
| .get(&output.store())? | ||
| .ok_or_else(|| anyhow!("UTXO entry not found for output: {:?}", output))?; |
| } else { | ||
| self | ||
| .get_output_info(*output)? | ||
| .ok_or_else(|| anyhow!("output {output} not found"))? |
|
@lifofifoX any thoughts on the review comments? |
3 participants
Attempts to fix #4208.
It currently has failing tests that I don't know how best to handle.