fix: warn when tx simulate discards existing signatures - #2696

Open
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures
Open

fix: warn when tx simulate discards existing signatures#2696
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures

Conversation

@Galmanus

Copy link
Copy Markdown

What

tx simulate silently dropped any signature already attached to the incoming envelope — unwrap_envelope_v1 destructures it and the re-wrapped result always carries an empty signature list. It now warns with the discarded count and the correct order of operations (simulate first, then sign).

Why

This is the concrete pain reported in #2549 (the author's follow-up comment): in a multisig flow, collected signatures vanish with no indication. The signatures are genuinely unusable after simulation — it rewrites the fee and Soroban transaction data, which invalidates them — so preserving them would be wrong; the missing piece was telling the user. This PR references #2549 rather than closing it: the issue also asks for repeated --sign-with-key and broader multisig UX, which are larger design decisions.

Testing

  • New signature_count helper in tx/xdr.rs with a unit test covering signed and unsigned envelopes; cargo test -p soroban-cli --lib tx::xdr::: 7 passed.
  • cargo clippy and cargo fmt --check clean.

CopilotAI balanced review requested due to automatic review settings August 22, 2026 10:45
@github-project-automationgithub-project-automationBot moved this to Backlog (Not Ready) in DevXAug 22, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Warns users when tx simulate invalidates and removes existing transaction signatures.

Changes:

  • Adds a helper to count envelope signatures.
  • Emits a simulation warning with the discarded count and correct workflow.
  • Adds unit coverage for signed and unsigned v1 envelopes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

FileDescription
cmd/soroban-cli/src/commands/tx/xdr.rsAdds signature counting and unit tests.
cmd/soroban-cli/src/commands/tx/simulate.rsWarns before dropping signatures during simulation.
Suppressed comments (1)

cmd/soroban-cli/src/commands/tx/simulate.rs:74

  • Add an integration test that passes a signed envelope through tx simulate and asserts the warning text/count on stderr and an unsigned simulated envelope on stdout. The added unit test only exercises signature_count, so it would still pass if this command never emitted the warning; command-level integration tests already live in cmd/crates/soroban-test/tests/it/integration/tx/general.rs:12-44, and repository guidance requires command behavior changes to be covered there.
 if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadcmd/soroban-cli/src/commands/tx/simulate.rs Outdated
`tx simulate` silently dropped any signature already attached to the
incoming envelope: `unwrap_envelope_v1` destructures the envelope and the
re-wrapped result always carries an empty signature list. The signatures
are genuinely unusable — simulation rewrites the fee and Soroban
transaction data, invalidating them — but dropping them without a word
leaves users of multisig flows wondering where their signatures went.
Emit a warning naming the count and the correct order (simulate first,
then sign), next to the existing fee-bump warning.
Ref stellar#2549
CopilotAI review requested due to automatic review settings September 1, 2026 23:05
@Galmanus
Galmanusforce-pushed the fix/2549-simulate-drops-signatures branch from 19aab5a to 341ea84CompareSeptember 1, 2026 23:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

match tx_env {
TransactionEnvelope::TxV0(e) => e.signatures.len(),
TransactionEnvelope::Tx(e) => e.signatures.len(),
TransactionEnvelope::TxFeeBump(e) => e.signatures.len(),
Comment on lines +77 to +81
if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog (Not Ready)

Development

Successfully merging this pull request may close these issues.

2 participants

@Galmanus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix: warn when tx simulate discards existing signatures - #2696

Open
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures
Open

fix: warn when tx simulate discards existing signatures#2696
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures

Conversation

@Galmanus

Copy link
Copy Markdown

What

tx simulate silently dropped any signature already attached to the incoming envelope — unwrap_envelope_v1 destructures it and the re-wrapped result always carries an empty signature list. It now warns with the discarded count and the correct order of operations (simulate first, then sign).

Why

This is the concrete pain reported in #2549 (the author's follow-up comment): in a multisig flow, collected signatures vanish with no indication. The signatures are genuinely unusable after simulation — it rewrites the fee and Soroban transaction data, which invalidates them — so preserving them would be wrong; the missing piece was telling the user. This PR references #2549 rather than closing it: the issue also asks for repeated --sign-with-key and broader multisig UX, which are larger design decisions.

Testing

  • New signature_count helper in tx/xdr.rs with a unit test covering signed and unsigned envelopes; cargo test -p soroban-cli --lib tx::xdr::: 7 passed.
  • cargo clippy and cargo fmt --check clean.

CopilotAI balanced review requested due to automatic review settings August 22, 2026 10:45
@github-project-automationgithub-project-automationBot moved this to Backlog (Not Ready) in DevXAug 22, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Warns users when tx simulate invalidates and removes existing transaction signatures.

Changes:

  • Adds a helper to count envelope signatures.
  • Emits a simulation warning with the discarded count and correct workflow.
  • Adds unit coverage for signed and unsigned v1 envelopes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

FileDescription
cmd/soroban-cli/src/commands/tx/xdr.rsAdds signature counting and unit tests.
cmd/soroban-cli/src/commands/tx/simulate.rsWarns before dropping signatures during simulation.
Suppressed comments (1)

cmd/soroban-cli/src/commands/tx/simulate.rs:74

  • Add an integration test that passes a signed envelope through tx simulate and asserts the warning text/count on stderr and an unsigned simulated envelope on stdout. The added unit test only exercises signature_count, so it would still pass if this command never emitted the warning; command-level integration tests already live in cmd/crates/soroban-test/tests/it/integration/tx/general.rs:12-44, and repository guidance requires command behavior changes to be covered there.
 if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadcmd/soroban-cli/src/commands/tx/simulate.rs Outdated
`tx simulate` silently dropped any signature already attached to the
incoming envelope: `unwrap_envelope_v1` destructures the envelope and the
re-wrapped result always carries an empty signature list. The signatures
are genuinely unusable — simulation rewrites the fee and Soroban
transaction data, invalidating them — but dropping them without a word
leaves users of multisig flows wondering where their signatures went.
Emit a warning naming the count and the correct order (simulate first,
then sign), next to the existing fee-bump warning.
Ref stellar#2549
CopilotAI review requested due to automatic review settings September 1, 2026 23:05
@Galmanus
Galmanusforce-pushed the fix/2549-simulate-drops-signatures branch from 19aab5a to 341ea84CompareSeptember 1, 2026 23:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

match tx_env {
TransactionEnvelope::TxV0(e) => e.signatures.len(),
TransactionEnvelope::Tx(e) => e.signatures.len(),
TransactionEnvelope::TxFeeBump(e) => e.signatures.len(),
Comment on lines +77 to +81
if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog (Not Ready)

Development

Successfully merging this pull request may close these issues.

2 participants

@Galmanus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix: warn when tx simulate discards existing signatures - #2696

Open
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures
Open

fix: warn when tx simulate discards existing signatures#2696
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures

Conversation

@Galmanus

Copy link
Copy Markdown

What

tx simulate silently dropped any signature already attached to the incoming envelope — unwrap_envelope_v1 destructures it and the re-wrapped result always carries an empty signature list. It now warns with the discarded count and the correct order of operations (simulate first, then sign).

Why

This is the concrete pain reported in #2549 (the author's follow-up comment): in a multisig flow, collected signatures vanish with no indication. The signatures are genuinely unusable after simulation — it rewrites the fee and Soroban transaction data, which invalidates them — so preserving them would be wrong; the missing piece was telling the user. This PR references #2549 rather than closing it: the issue also asks for repeated --sign-with-key and broader multisig UX, which are larger design decisions.

Testing

  • New signature_count helper in tx/xdr.rs with a unit test covering signed and unsigned envelopes; cargo test -p soroban-cli --lib tx::xdr::: 7 passed.
  • cargo clippy and cargo fmt --check clean.

CopilotAI balanced review requested due to automatic review settings August 22, 2026 10:45
@github-project-automationgithub-project-automationBot moved this to Backlog (Not Ready) in DevXAug 22, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Warns users when tx simulate invalidates and removes existing transaction signatures.

Changes:

  • Adds a helper to count envelope signatures.
  • Emits a simulation warning with the discarded count and correct workflow.
  • Adds unit coverage for signed and unsigned v1 envelopes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

FileDescription
cmd/soroban-cli/src/commands/tx/xdr.rsAdds signature counting and unit tests.
cmd/soroban-cli/src/commands/tx/simulate.rsWarns before dropping signatures during simulation.
Suppressed comments (1)

cmd/soroban-cli/src/commands/tx/simulate.rs:74

  • Add an integration test that passes a signed envelope through tx simulate and asserts the warning text/count on stderr and an unsigned simulated envelope on stdout. The added unit test only exercises signature_count, so it would still pass if this command never emitted the warning; command-level integration tests already live in cmd/crates/soroban-test/tests/it/integration/tx/general.rs:12-44, and repository guidance requires command behavior changes to be covered there.
 if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadcmd/soroban-cli/src/commands/tx/simulate.rs Outdated
`tx simulate` silently dropped any signature already attached to the
incoming envelope: `unwrap_envelope_v1` destructures the envelope and the
re-wrapped result always carries an empty signature list. The signatures
are genuinely unusable — simulation rewrites the fee and Soroban
transaction data, invalidating them — but dropping them without a word
leaves users of multisig flows wondering where their signatures went.
Emit a warning naming the count and the correct order (simulate first,
then sign), next to the existing fee-bump warning.
Ref stellar#2549
CopilotAI review requested due to automatic review settings September 1, 2026 23:05
@Galmanus
Galmanusforce-pushed the fix/2549-simulate-drops-signatures branch from 19aab5a to 341ea84CompareSeptember 1, 2026 23:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

match tx_env {
TransactionEnvelope::TxV0(e) => e.signatures.len(),
TransactionEnvelope::Tx(e) => e.signatures.len(),
TransactionEnvelope::TxFeeBump(e) => e.signatures.len(),
Comment on lines +77 to +81
if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog (Not Ready)

Development

Successfully merging this pull request may close these issues.

2 participants

@Galmanus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix: warn when tx simulate discards existing signatures - #2696

Open
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures
Open

fix: warn when tx simulate discards existing signatures#2696
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures

Conversation

@Galmanus

Copy link
Copy Markdown

What

tx simulate silently dropped any signature already attached to the incoming envelope — unwrap_envelope_v1 destructures it and the re-wrapped result always carries an empty signature list. It now warns with the discarded count and the correct order of operations (simulate first, then sign).

Why

This is the concrete pain reported in #2549 (the author's follow-up comment): in a multisig flow, collected signatures vanish with no indication. The signatures are genuinely unusable after simulation — it rewrites the fee and Soroban transaction data, which invalidates them — so preserving them would be wrong; the missing piece was telling the user. This PR references #2549 rather than closing it: the issue also asks for repeated --sign-with-key and broader multisig UX, which are larger design decisions.

Testing

  • New signature_count helper in tx/xdr.rs with a unit test covering signed and unsigned envelopes; cargo test -p soroban-cli --lib tx::xdr::: 7 passed.
  • cargo clippy and cargo fmt --check clean.

CopilotAI balanced review requested due to automatic review settings August 22, 2026 10:45
@github-project-automationgithub-project-automationBot moved this to Backlog (Not Ready) in DevXAug 22, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Warns users when tx simulate invalidates and removes existing transaction signatures.

Changes:

  • Adds a helper to count envelope signatures.
  • Emits a simulation warning with the discarded count and correct workflow.
  • Adds unit coverage for signed and unsigned v1 envelopes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

FileDescription
cmd/soroban-cli/src/commands/tx/xdr.rsAdds signature counting and unit tests.
cmd/soroban-cli/src/commands/tx/simulate.rsWarns before dropping signatures during simulation.
Suppressed comments (1)

cmd/soroban-cli/src/commands/tx/simulate.rs:74

  • Add an integration test that passes a signed envelope through tx simulate and asserts the warning text/count on stderr and an unsigned simulated envelope on stdout. The added unit test only exercises signature_count, so it would still pass if this command never emitted the warning; command-level integration tests already live in cmd/crates/soroban-test/tests/it/integration/tx/general.rs:12-44, and repository guidance requires command behavior changes to be covered there.
 if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadcmd/soroban-cli/src/commands/tx/simulate.rs Outdated
`tx simulate` silently dropped any signature already attached to the
incoming envelope: `unwrap_envelope_v1` destructures the envelope and the
re-wrapped result always carries an empty signature list. The signatures
are genuinely unusable — simulation rewrites the fee and Soroban
transaction data, invalidating them — but dropping them without a word
leaves users of multisig flows wondering where their signatures went.
Emit a warning naming the count and the correct order (simulate first,
then sign), next to the existing fee-bump warning.
Ref stellar#2549
CopilotAI review requested due to automatic review settings September 1, 2026 23:05
@Galmanus
Galmanusforce-pushed the fix/2549-simulate-drops-signatures branch from 19aab5a to 341ea84CompareSeptember 1, 2026 23:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

match tx_env {
TransactionEnvelope::TxV0(e) => e.signatures.len(),
TransactionEnvelope::Tx(e) => e.signatures.len(),
TransactionEnvelope::TxFeeBump(e) => e.signatures.len(),
Comment on lines +77 to +81
if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog (Not Ready)

Development

Successfully merging this pull request may close these issues.

2 participants

@Galmanus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fix: warn when tx simulate discards existing signatures - #2696

Open
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures
Open

fix: warn when tx simulate discards existing signatures#2696
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures

Conversation

@Galmanus

Copy link
Copy Markdown

What

tx simulate silently dropped any signature already attached to the incoming envelope — unwrap_envelope_v1 destructures it and the re-wrapped result always carries an empty signature list. It now warns with the discarded count and the correct order of operations (simulate first, then sign).

Why

This is the concrete pain reported in #2549 (the author's follow-up comment): in a multisig flow, collected signatures vanish with no indication. The signatures are genuinely unusable after simulation — it rewrites the fee and Soroban transaction data, which invalidates them — so preserving them would be wrong; the missing piece was telling the user. This PR references #2549 rather than closing it: the issue also asks for repeated --sign-with-key and broader multisig UX, which are larger design decisions.

Testing

  • New signature_count helper in tx/xdr.rs with a unit test covering signed and unsigned envelopes; cargo test -p soroban-cli --lib tx::xdr::: 7 passed.
  • cargo clippy and cargo fmt --check clean.

CopilotAI balanced review requested due to automatic review settings August 22, 2026 10:45
@github-project-automationgithub-project-automationBot moved this to Backlog (Not Ready) in DevXAug 22, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Warns users when tx simulate invalidates and removes existing transaction signatures.

Changes:

  • Adds a helper to count envelope signatures.
  • Emits a simulation warning with the discarded count and correct workflow.
  • Adds unit coverage for signed and unsigned v1 envelopes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

FileDescription
cmd/soroban-cli/src/commands/tx/xdr.rsAdds signature counting and unit tests.
cmd/soroban-cli/src/commands/tx/simulate.rsWarns before dropping signatures during simulation.
Suppressed comments (1)

cmd/soroban-cli/src/commands/tx/simulate.rs:74

  • Add an integration test that passes a signed envelope through tx simulate and asserts the warning text/count on stderr and an unsigned simulated envelope on stdout. The added unit test only exercises signature_count, so it would still pass if this command never emitted the warning; command-level integration tests already live in cmd/crates/soroban-test/tests/it/integration/tx/general.rs:12-44, and repository guidance requires command behavior changes to be covered there.
 if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadcmd/soroban-cli/src/commands/tx/simulate.rs Outdated
`tx simulate` silently dropped any signature already attached to the
incoming envelope: `unwrap_envelope_v1` destructures the envelope and the
re-wrapped result always carries an empty signature list. The signatures
are genuinely unusable — simulation rewrites the fee and Soroban
transaction data, invalidating them — but dropping them without a word
leaves users of multisig flows wondering where their signatures went.
Emit a warning naming the count and the correct order (simulate first,
then sign), next to the existing fee-bump warning.
Ref stellar#2549
CopilotAI review requested due to automatic review settings September 1, 2026 23:05
@Galmanus
Galmanusforce-pushed the fix/2549-simulate-drops-signatures branch from 19aab5a to 341ea84CompareSeptember 1, 2026 23:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

match tx_env {
TransactionEnvelope::TxV0(e) => e.signatures.len(),
TransactionEnvelope::Tx(e) => e.signatures.len(),
TransactionEnvelope::TxFeeBump(e) => e.signatures.len(),
Comment on lines +77 to +81
if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog (Not Ready)

Development

Successfully merging this pull request may close these issues.

2 participants

@Galmanus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix: warn when tx simulate discards existing signatures - #2696

Open
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures
Open

fix: warn when tx simulate discards existing signatures#2696
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures

Conversation

@Galmanus

Copy link
Copy Markdown

What

tx simulate silently dropped any signature already attached to the incoming envelope — unwrap_envelope_v1 destructures it and the re-wrapped result always carries an empty signature list. It now warns with the discarded count and the correct order of operations (simulate first, then sign).

Why

This is the concrete pain reported in #2549 (the author's follow-up comment): in a multisig flow, collected signatures vanish with no indication. The signatures are genuinely unusable after simulation — it rewrites the fee and Soroban transaction data, which invalidates them — so preserving them would be wrong; the missing piece was telling the user. This PR references #2549 rather than closing it: the issue also asks for repeated --sign-with-key and broader multisig UX, which are larger design decisions.

Testing

  • New signature_count helper in tx/xdr.rs with a unit test covering signed and unsigned envelopes; cargo test -p soroban-cli --lib tx::xdr::: 7 passed.
  • cargo clippy and cargo fmt --check clean.

CopilotAI balanced review requested due to automatic review settings August 22, 2026 10:45
@github-project-automationgithub-project-automationBot moved this to Backlog (Not Ready) in DevXAug 22, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Warns users when tx simulate invalidates and removes existing transaction signatures.

Changes:

  • Adds a helper to count envelope signatures.
  • Emits a simulation warning with the discarded count and correct workflow.
  • Adds unit coverage for signed and unsigned v1 envelopes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

FileDescription
cmd/soroban-cli/src/commands/tx/xdr.rsAdds signature counting and unit tests.
cmd/soroban-cli/src/commands/tx/simulate.rsWarns before dropping signatures during simulation.
Suppressed comments (1)

cmd/soroban-cli/src/commands/tx/simulate.rs:74

  • Add an integration test that passes a signed envelope through tx simulate and asserts the warning text/count on stderr and an unsigned simulated envelope on stdout. The added unit test only exercises signature_count, so it would still pass if this command never emitted the warning; command-level integration tests already live in cmd/crates/soroban-test/tests/it/integration/tx/general.rs:12-44, and repository guidance requires command behavior changes to be covered there.
 if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadcmd/soroban-cli/src/commands/tx/simulate.rs Outdated
`tx simulate` silently dropped any signature already attached to the
incoming envelope: `unwrap_envelope_v1` destructures the envelope and the
re-wrapped result always carries an empty signature list. The signatures
are genuinely unusable — simulation rewrites the fee and Soroban
transaction data, invalidating them — but dropping them without a word
leaves users of multisig flows wondering where their signatures went.
Emit a warning naming the count and the correct order (simulate first,
then sign), next to the existing fee-bump warning.
Ref stellar#2549
CopilotAI review requested due to automatic review settings September 1, 2026 23:05
@Galmanus
Galmanusforce-pushed the fix/2549-simulate-drops-signatures branch from 19aab5a to 341ea84CompareSeptember 1, 2026 23:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

match tx_env {
TransactionEnvelope::TxV0(e) => e.signatures.len(),
TransactionEnvelope::Tx(e) => e.signatures.len(),
TransactionEnvelope::TxFeeBump(e) => e.signatures.len(),
Comment on lines +77 to +81
if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog (Not Ready)

Development

Successfully merging this pull request may close these issues.

2 participants

@Galmanus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix: warn when tx simulate discards existing signatures - #2696

Open
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures
Open

fix: warn when tx simulate discards existing signatures#2696
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures

Conversation

@Galmanus

Copy link
Copy Markdown

What

tx simulate silently dropped any signature already attached to the incoming envelope — unwrap_envelope_v1 destructures it and the re-wrapped result always carries an empty signature list. It now warns with the discarded count and the correct order of operations (simulate first, then sign).

Why

This is the concrete pain reported in #2549 (the author's follow-up comment): in a multisig flow, collected signatures vanish with no indication. The signatures are genuinely unusable after simulation — it rewrites the fee and Soroban transaction data, which invalidates them — so preserving them would be wrong; the missing piece was telling the user. This PR references #2549 rather than closing it: the issue also asks for repeated --sign-with-key and broader multisig UX, which are larger design decisions.

Testing

  • New signature_count helper in tx/xdr.rs with a unit test covering signed and unsigned envelopes; cargo test -p soroban-cli --lib tx::xdr::: 7 passed.
  • cargo clippy and cargo fmt --check clean.

CopilotAI balanced review requested due to automatic review settings August 22, 2026 10:45
@github-project-automationgithub-project-automationBot moved this to Backlog (Not Ready) in DevXAug 22, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Warns users when tx simulate invalidates and removes existing transaction signatures.

Changes:

  • Adds a helper to count envelope signatures.
  • Emits a simulation warning with the discarded count and correct workflow.
  • Adds unit coverage for signed and unsigned v1 envelopes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

FileDescription
cmd/soroban-cli/src/commands/tx/xdr.rsAdds signature counting and unit tests.
cmd/soroban-cli/src/commands/tx/simulate.rsWarns before dropping signatures during simulation.
Suppressed comments (1)

cmd/soroban-cli/src/commands/tx/simulate.rs:74

  • Add an integration test that passes a signed envelope through tx simulate and asserts the warning text/count on stderr and an unsigned simulated envelope on stdout. The added unit test only exercises signature_count, so it would still pass if this command never emitted the warning; command-level integration tests already live in cmd/crates/soroban-test/tests/it/integration/tx/general.rs:12-44, and repository guidance requires command behavior changes to be covered there.
 if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadcmd/soroban-cli/src/commands/tx/simulate.rs Outdated
`tx simulate` silently dropped any signature already attached to the
incoming envelope: `unwrap_envelope_v1` destructures the envelope and the
re-wrapped result always carries an empty signature list. The signatures
are genuinely unusable — simulation rewrites the fee and Soroban
transaction data, invalidating them — but dropping them without a word
leaves users of multisig flows wondering where their signatures went.
Emit a warning naming the count and the correct order (simulate first,
then sign), next to the existing fee-bump warning.
Ref stellar#2549
CopilotAI review requested due to automatic review settings September 1, 2026 23:05
@Galmanus
Galmanusforce-pushed the fix/2549-simulate-drops-signatures branch from 19aab5a to 341ea84CompareSeptember 1, 2026 23:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

match tx_env {
TransactionEnvelope::TxV0(e) => e.signatures.len(),
TransactionEnvelope::Tx(e) => e.signatures.len(),
TransactionEnvelope::TxFeeBump(e) => e.signatures.len(),
Comment on lines +77 to +81
if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog (Not Ready)

Development

Successfully merging this pull request may close these issues.

2 participants

@Galmanus
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fix: warn when tx simulate discards existing signatures - #2696

Open
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures
Open

fix: warn when tx simulate discards existing signatures#2696
Galmanus wants to merge 2 commits into
stellar:mainfrom
Galmanus:fix/2549-simulate-drops-signatures

Conversation

@Galmanus

Copy link
Copy Markdown

What

tx simulate silently dropped any signature already attached to the incoming envelope — unwrap_envelope_v1 destructures it and the re-wrapped result always carries an empty signature list. It now warns with the discarded count and the correct order of operations (simulate first, then sign).

Why

This is the concrete pain reported in #2549 (the author's follow-up comment): in a multisig flow, collected signatures vanish with no indication. The signatures are genuinely unusable after simulation — it rewrites the fee and Soroban transaction data, which invalidates them — so preserving them would be wrong; the missing piece was telling the user. This PR references #2549 rather than closing it: the issue also asks for repeated --sign-with-key and broader multisig UX, which are larger design decisions.

Testing

  • New signature_count helper in tx/xdr.rs with a unit test covering signed and unsigned envelopes; cargo test -p soroban-cli --lib tx::xdr::: 7 passed.
  • cargo clippy and cargo fmt --check clean.

CopilotAI balanced review requested due to automatic review settings August 22, 2026 10:45
@github-project-automationgithub-project-automationBot moved this to Backlog (Not Ready) in DevXAug 22, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Warns users when tx simulate invalidates and removes existing transaction signatures.

Changes:

  • Adds a helper to count envelope signatures.
  • Emits a simulation warning with the discarded count and correct workflow.
  • Adds unit coverage for signed and unsigned v1 envelopes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

FileDescription
cmd/soroban-cli/src/commands/tx/xdr.rsAdds signature counting and unit tests.
cmd/soroban-cli/src/commands/tx/simulate.rsWarns before dropping signatures during simulation.
Suppressed comments (1)

cmd/soroban-cli/src/commands/tx/simulate.rs:74

  • Add an integration test that passes a signed envelope through tx simulate and asserts the warning text/count on stderr and an unsigned simulated envelope on stdout. The added unit test only exercises signature_count, so it would still pass if this command never emitted the warning; command-level integration tests already live in cmd/crates/soroban-test/tests/it/integration/tx/general.rs:12-44, and repository guidance requires command behavior changes to be covered there.
 if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadcmd/soroban-cli/src/commands/tx/simulate.rs Outdated
`tx simulate` silently dropped any signature already attached to the
incoming envelope: `unwrap_envelope_v1` destructures the envelope and the
re-wrapped result always carries an empty signature list. The signatures
are genuinely unusable — simulation rewrites the fee and Soroban
transaction data, invalidating them — but dropping them without a word
leaves users of multisig flows wondering where their signatures went.
Emit a warning naming the count and the correct order (simulate first,
then sign), next to the existing fee-bump warning.
Ref stellar#2549
CopilotAI review requested due to automatic review settings September 1, 2026 23:05
@Galmanus
Galmanusforce-pushed the fix/2549-simulate-drops-signatures branch from 19aab5a to 341ea84CompareSeptember 1, 2026 23:05

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

match tx_env {
TransactionEnvelope::TxV0(e) => e.signatures.len(),
TransactionEnvelope::Tx(e) => e.signatures.len(),
TransactionEnvelope::TxFeeBump(e) => e.signatures.len(),
Comment on lines +77 to +81
if discarded > 0 {
print.warnln(format!(
"Discarding {discarded} existing signature(s): simulation changes the \
transaction's fee and resources, which invalidates prior signatures. \
Simulate first, then sign the result."
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog (Not Ready)

Development

Successfully merging this pull request may close these issues.

2 participants

@Galmanus