Skip to content

feat: escrow account sweeping#220

Merged
danwt merged 20 commits intomain-dymfrom
kirill/sweeping
Aug 14, 2025
Merged

feat: escrow account sweeping#220
danwt merged 20 commits intomain-dymfrom
kirill/sweeping

Conversation

@keruch
Copy link
Copy Markdown

@keruch keruch commented Aug 8, 2025

@keruch keruch self-assigned this Aug 8, 2025
Comment on lines 207 to 247
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

there was a bug when messages was overwriting itself

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I removed must_match and used escrow_public + address_prefix so I can easily use validate_pskts in the demo

Comment on lines 333 to 339
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Important part!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I added validation to the demo (without any hub call though)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I created this function to build WithdrawFXG without any hub calls

.map_err(|e| eyre::eyre!("Estimate TX mass: {e}"))?;

// Apply TX mass multiplier and feerate
let tx_fee = (tx_mass as f64 * TX_MASS_MULTIPLIER * feerate).round() as u64;
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I added feerate, it closes #112

Ok((anchor_input, escrow_inputs))
}

fn estimate_mass(
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I updated estimate_fee (now it is estimate_mass) to use a different MassCalculator which kaspa is using in the tx generator. I found it more accurate and I stopped getting insufficient fee errors.

Comment on lines +270 to +295
let tx_id = pskt.calculate_id();

// If there are no messages and payload is empty, then the PSKT
// is a sweeping tx which does not spend the anchor
let tx_type = if expected_messages.is_empty() && pskt.global.payload.is_none() {
info!("PSKT is a sweeping tx: {tx_id}");
TxType::Sweeping
} else {
info!("PSKT is a withdrawal tx: {tx_id}");
TxType::Withdrawal
};

let ix = validate_pskt_application_semantics(
&pskt,
must_spend,
tx_type,
expected_messages,
escrow_public,
address_prefix,
)?;

match tx_type {
// In case of the sweeping tx, try to spend the anchor in the next PSKT
TxType::Sweeping => Ok(must_spend),
TxType::Withdrawal => Ok(TransactionOutpoint::new(pskt.calculate_id(), ix)),
}
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

important to check!!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@keruch keruch marked this pull request as ready for review August 12, 2025 18:32
let args = Args::parse();
let mut args = Args::parse();

args.wallet_secret = Some("123456qwe".to_string());
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

lol

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

security is my second name

danwt
danwt previously approved these changes Aug 13, 2025
Copy link
Copy Markdown

@danwt danwt left a comment

Choose a reason for hiding this comment

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

Great work! Just a few clarifying questions

Comment on lines +14 to 24
let utxos = rpc
.get_utxos_by_addresses(vec![addr.clone()])
.await
.map_err(|e| Error::Custom(format!("Getting balance for address: {e}")))?;
.map_err(|e| Error::Custom(format!("Getting UTXOs for address: {e}")))?;

let num = utxos.len();
let balance: u64 = utxos.into_iter().map(|u| u.utxo_entry.amount).sum();

info!("{} has {} UTXOs and {} balance", source, num, balance);

info!("{} balance: {}", source, balance);
Ok(balance)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@keruch what was wrong with the old way?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

nothing, I just wanted to know the number of UTXOs

// Add this small priproty fee to every sweeping TX to ensure the sufficient fee even if there
// is one validator. 3_000 is a magic number.
// TODO: make it configurable?
pub const RELAYER_SWEEPING_PRIORITY_FEE: u64 = 3_000;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is this also going to work on mainnet?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

not sure, that's why I think it's worth making it configurable. 3000 is just what I came up with while testing.

Comment on lines +391 to 402
let tx_hash = h512_to_cosmos_hash(outcome.transaction_id).encode_hex_upper::<String>();

if !outcome.executed {
return Err(eyre::eyre!(
"Indicate progress failed, TX was not executed on-chain, tx hash: {tx_hash}"
));
}

info!(
"Dymension, indicated progress on hub: {:?}, outcome: {:?}",
fxg.progress_indication, outcome
"Dymension, indicated progress on hub: {:?}, outcome: {:?}, tx hash: {:?}",
fxg.progress_indication, outcome, tx_hash,
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚀

let messages = {
let sweep_count = sweeping_bundle.as_ref().map_or(0, |b| b.0.len());
let mut messages = Vec::with_capacity(sweep_count + valid_msgs.len());
messages.extend(vec![Vec::new(); sweep_count]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what is the Vec::new() here for?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

vec![V; N] (with semicolon) is a macro to create a vector of V repeated N times. so with vec![Vec::new(); sweep_count] I create a vector of sweep_count empty vectors, ie [[], [], [], ...].

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ah i see!

_ => {}
}

let payload_expect = MessageIDs::from(expected_messages).to_bytes();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the payload check still works properly? i.e. sweep is checked to be empty?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yes! it's here

let tx_type = if expected_messages.is_empty() && pskt.global.payload.is_none() {
info!("PSKT is a sweeping tx: {tx_id}");
TxType::Sweeping
} else {
info!("PSKT is a withdrawal tx: {tx_id}");
TxType::Withdrawal
};


// If there are no messages and payload is empty, then the PSKT
// is a sweeping tx which does not spend the anchor
let tx_type = if expected_messages.is_empty() && pskt.global.payload.is_none() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is this clause strictly needed?

&& pskt.global.payload.is_none()
it's confusing me

i think the code will still be correct without this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I wanted to distinguish between sweeping and withdrawal TXs in the validation. In case of sweeping, we don't want the anchor to be an input. In case of withdrawal, we do.

// Check that the PSKT is a sweeping tx or a withdrawal tx.
// If it is a sweeping tx, then the anchor must not be spent.
// If it is a withdrawal tx, then the anchor must be spent.
match (tx_type, anchor_found) {
(TxType::Sweeping, true) => {
return Err(ValidationError::AnchorSpent { o: current_anchor });
}
(TxType::Withdrawal, false) => {
return Err(ValidationError::AnchorNotFound { o: current_anchor });
}
_ => {}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Right, but I mean, you can check expected messages empty, there is no need to check the payload too in this if statement?
It makes it harder to understand IMO because then there is the payload correctness check in the next function clal

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

the payload correctness check in the next function

now I got it! fixed

danwt and others added 6 commits August 13, 2025 13:05
- Changed PopulatedInput to include optional redeem_script as third element
- Keep signature_script empty for unsigned transactions (as it should be)
- Store redeem_script separately and only add to PSKT when needed
- Update all usages to handle the new tuple structure

This fixes the semantic confusion where redeem_script was incorrectly
stored in the signature_script field of TransactionInput.
@danwt danwt merged commit 5af7f5a into main-dym Aug 14, 2025
9 of 62 checks passed
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