Refactor SDK args and sigs #4816

grarco · 2025-09-10T09:45:21Z

Describe your changes

Closes #3901 .
Closes #3883.

Reworks the sdk to address the followings:

Takes the tx arguments relative to wrapping into an optional separate type (to better control whether a tx should be wrapped or not during the build phase)
Improves the dump and dry-run args by making the enum (inner or wrapper)
Removes the wrapper signing data from SigningTxData and move it to a new SigningWrapperData which contains all the signing data for a batch
Simplified the interface of aux_signing_data
The tx builders will now return a separate signing data variant based on wether the tx was wrapped or not
Simplified the logic when handling the signatures of a dumped transaction
Some new safety checks on operations, like avoiding casting a wrapper tx to a raw one or submitting a non-wrapped transaction
Removes some useless dispatcher functions
Removes explicit handling of the MASP address as a tx owner. The owner should not be specified in these cases
Minor changes to the interface of the build_batch function
Removed the owner for reveal_pk (no signature is needed for this transaction)

Checklist before merging

If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
- This will require 2 reviewers to approve the changes
If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
- Relevant PR if applies:
If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
- Relevant PR if applies:

grarco · 2025-09-19T14:40:31Z

crates/sdk/src/signing.rs

+                }
+                // NOTE: the disposable gas_payer is always overridden by
+                // signing inputs
+            } else if disposable_gas_payer {


Technically if a disposable gas payer is required no signer should be provided since it's not needed, grows the size of the tx (and the gas required) and might leak information. Still, I've maintained the same logic we've had in place for a while for the wrapper signer: if the user specifically passes a signing key we still produce a signature for that (i.e. the user is still in control even though we try to guide them)

mergify · 2025-09-19T15:04:05Z

🧪 CI Insights

Here's what we observed from your CI run for 3394fd7.

✅ Passed Jobs With Interesting Signals

Pipeline	Job	Signal	Health on `main`	Retries	🔍 CI Insights	📄 Logs
`Lint 🧹 Build 🛠️ Test 🚦`	`test-e2e (0)`	Base branch is healthy, but retries were needed. Could be early signs of flakiness 👀		`1`	View	View
`Lint 🧹 Build 🛠️ Test 🚦`	`test-integration`	Base branch is broken, but the job passed. Looks like this might be a real fix 💪		`0`	View	View

tzemanovic · 2025-09-22T09:01:43Z

.changelog/unreleased/SDK/4816-wrapper-sigs.md

@@ -0,0 +1,2 @@
+- Reworked SDK wrapping and signatures.


if we do take this PR I think this should have a bit more details, especially about the breaking changes

Made the message more specific in 35a691c

tzemanovic · 2025-09-22T09:16:47Z

crates/sdk/src/eth_bridge/bridge_pool.rs

 use namada_core::voting_power::FractionalVotingPower;
 use namada_ethereum_bridge::storage::bridge_pool::get_pending_key;
 use namada_io::{Client, Io, display, display_line, edisplay_line};
 use namada_token::Amount;


unrelated to this pr but I'm wondering if we should perhaps drop all this eth bridge stuff

crates/sdk/src/signing.rs

tzemanovic · 2025-09-22T09:37:32Z

crates/sdk/src/tx.rs

+        .await?;
+
+        (SigningData::Inner(signing_data), None)
+    };


look like this new block is the same in many tx types, maybe we can re-use it via some helper fn?

Right, refactored in 4e33435

grarco · 2025-09-24T12:42:47Z

@tzemanovic I also fixed a bug for which we were attaching empty auth sections to the transactions (you can see it in de7eda5). That's what was making the e2e tests fail (unless something else comes up in this CI run)

tzemanovic · 2025-09-24T14:46:40Z

@tzemanovic I also fixed a bug for which we were attaching empty auth sections to the transactions (you can see it in de7eda5). That's what was making the e2e tests fail (unless something else comes up in this CI run)

thx! I'm just merging #4832 which I think will introduce some conflicts if you don't mind rebasing this after

grarco · 2025-09-24T14:50:05Z

Yes go ahead, I need to fix some more broken tests in the meantime

grarco · 2025-09-26T15:25:51Z

@tzemanovic I think this is read for a final re-review. Mind that I changed the following:

the logic of inner_tx_signer has been reverted to the previous one where the public keys take precedence over the default_signer instead of the two being merged together
build_batch has been updated to allow batching txs that are already batch (it was previously limited to a single inner transaction)

.changelog/unreleased/SDK/4816-wrapper-sigs.md

tzemanovic · 2025-10-01T13:48:45Z

I've just added a tiny commit to fix the CI changelog check

codecov · 2025-10-01T15:48:27Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

(cherry picked from commit 73c9efd)

Refactor SDK args and sigs (backport #4816)

(cherry picked from commit 73c9efd)

…pr-4816 Refactor SDK args and sigs (backport #4816)

grarco added SDK breaking:SDK SDK breaking change labels Sep 10, 2025

grarco force-pushed the grarco/wrapper-sigs branch 2 times, most recently from 82edf81 to cbac096 Compare September 15, 2025 12:07

grarco added a commit that referenced this pull request Sep 19, 2025

Changelog #4816

3e56f75

grarco force-pushed the grarco/wrapper-sigs branch from 98315b4 to 3e56f75 Compare September 19, 2025 14:27

grarco marked this pull request as ready for review September 19, 2025 14:28

grarco requested a review from tzemanovic September 19, 2025 14:34

grarco commented Sep 19, 2025

View reviewed changes

github-actions bot added the breaking:api public API breaking change label Sep 19, 2025

tzemanovic reviewed Sep 22, 2025

View reviewed changes

crates/sdk/src/signing.rs Outdated Show resolved Hide resolved

tzemanovic reviewed Sep 22, 2025

View reviewed changes

crates/sdk/src/signing.rs Show resolved Hide resolved

tzemanovic reviewed Sep 22, 2025

View reviewed changes

grarco added a commit that referenced this pull request Sep 25, 2025

Changelog #4816

8a33594

grarco force-pushed the grarco/wrapper-sigs branch from 35a691c to 685a151 Compare September 25, 2025 13:47

grarco added a commit that referenced this pull request Sep 26, 2025

Changelog #4816

a7623af

grarco force-pushed the grarco/wrapper-sigs branch from 782b04f to a7623af Compare September 26, 2025 13:50

grarco added a commit that referenced this pull request Sep 26, 2025

Changelog #4816

12dc976

grarco force-pushed the grarco/wrapper-sigs branch from a7623af to 12dc976 Compare September 26, 2025 14:14

grarco requested a review from tzemanovic September 26, 2025 15:25

grarco and others added 3 commits September 30, 2025 09:56

Refactors the signatures structures of the SDK

1e39180

Merges the two dry-run args into a single one

dd0d9bc

Merges the two dump-tx args into a single one

5427a95

grarco added 2 commits September 30, 2025 11:38

Upgrades build_batch to support batched transactions

0fb380f

Changelog #4816

73c9efd

grarco force-pushed the grarco/wrapper-sigs branch from ec0d850 to 73c9efd Compare September 30, 2025 09:38

tzemanovic reviewed Oct 1, 2025

View reviewed changes

.changelog/unreleased/SDK/4816-wrapper-sigs.md Outdated Show resolved Hide resolved

Fixes typo in changelog

603b826

tzemanovic approved these changes Oct 1, 2025

View reviewed changes

ci: add missing SDK issue category

3394fd7

tzemanovic added backport-libs-0.251 Backport libraries to 0.251 maintenance branch backport-201.0 Backport to app 201.0 maintenance branch merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass labels Oct 1, 2025

mergify bot added the queued label Oct 1, 2025

mergify bot added a commit that referenced this pull request Oct 1, 2025

Merge of #4816

93523bc

mergify bot mentioned this pull request Oct 1, 2025

merge queue: embarking main (58a926b), #4877 and #4816 together #4881

Closed

mergify bot added a commit that referenced this pull request Oct 1, 2025

Merge of #4816

32bef2f

mergify bot mentioned this pull request Oct 1, 2025

merge queue: embarking main (58a926b) and #4816 together #4882

Closed

mergify bot added a commit that referenced this pull request Oct 1, 2025

Merge of #4816

7301bb3

mergify bot added a commit that referenced this pull request Oct 1, 2025

Merge of #4816

fa26022

mergify bot merged commit b3b8e63 into main Oct 1, 2025
45 of 46 checks passed

mergify bot deleted the grarco/wrapper-sigs branch October 1, 2025 17:08

mergify bot removed the queued label Oct 1, 2025

mergify bot pushed a commit that referenced this pull request Oct 1, 2025

Changelog #4816

e737bcb

(cherry picked from commit 73c9efd)

mergify bot mentioned this pull request Oct 1, 2025

Refactor SDK args and sigs (backport #4816) #4886

Merged

3 tasks

mergify bot pushed a commit that referenced this pull request Oct 1, 2025

Changelog #4816

33af0fc

(cherry picked from commit 73c9efd)

mergify bot mentioned this pull request Oct 1, 2025

Refactor SDK args and sigs (backport #4816) #4887

Merged

3 tasks

mergify bot added a commit that referenced this pull request Oct 1, 2025

Merge pull request #4886 from namada-net/mergify/bp/maint-201.0/pr-4816

7f27de7

Refactor SDK args and sigs (backport #4816)

tzemanovic mentioned this pull request Oct 8, 2025

Dry run with mock sigs #4832

Merged

3 tasks

tzemanovic pushed a commit that referenced this pull request Oct 8, 2025

Changelog #4816

7762490

(cherry picked from commit 73c9efd)

mergify bot added a commit that referenced this pull request Oct 8, 2025

Merge pull request #4887 from namada-net/mergify/bp/maint-libs-0.251/…

2a560e2

…pr-4816 Refactor SDK args and sigs (backport #4816)

Refactor SDK args and sigs #4816

Refactor SDK args and sigs #4816

Uh oh!

Conversation

grarco commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Checklist before merging

Uh oh!

grarco Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🧪 CI Insights

✅ Passed Jobs With Interesting Signals

Uh oh!

tzemanovic Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

grarco Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

tzemanovic Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tzemanovic Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grarco Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

grarco commented Sep 24, 2025

Uh oh!

tzemanovic commented Sep 24, 2025

Uh oh!

grarco commented Sep 24, 2025

Uh oh!

grarco commented Sep 26, 2025

Uh oh!

Uh oh!

tzemanovic commented Oct 1, 2025

Uh oh!

codecov bot commented Oct 1, 2025

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

grarco commented Sep 10, 2025 •

edited

Loading

mergify bot commented Sep 19, 2025 •

edited

Loading

tzemanovic Sep 22, 2025 •

edited

Loading