Skip to content

Conversation

@grarco
Copy link
Collaborator

@grarco grarco commented Nov 3, 2025

Describe your changes

Closes #4074.

Removes the speculative shielded context and the shielded-sync cli command. The shielded-sync operations is now embedded directly in the cli commands that need it:

  • Shielded transfer
  • Unshielding transfer
  • IBC transfer (when the source is shielded)
  • Balance query (for shielded owners)
  • Shielded rewards estimate

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
  • 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 grarco added cli UX client MASP breaking:cli command line breaking change SDK breaking:SDK SDK breaking change labels Nov 3, 2025
@github-actions github-actions bot added the breaking:api public API breaking change label Nov 3, 2025
@grarco grarco requested a review from sug0 November 3, 2025 16:01
@mergify
Copy link
Contributor

mergify bot commented Nov 3, 2025

🧪 CI Insights

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

🟢 All jobs passed!

But CI Insights is watching 👀

@grarco grarco force-pushed the grarco/delete-speculative-context branch from 05e0230 to ec5d786 Compare November 4, 2025 14:56
.subcommand(QueryEffNativeSupply::def().display_order(5))
.subcommand(QueryStakingRewardsRate::def().display_order(5))
// Actions
.subcommand(ShieldedSync::def().display_order(6))
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we bring back the standalone shielded sync command?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dropped the commit that removed this command, it's back now, I will also update the docs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@grarco grarco force-pushed the grarco/delete-speculative-context branch from 51fd175 to 9116705 Compare November 11, 2025 15:38
@sug0 sug0 self-requested a review November 11, 2025 18:43
@sug0
Copy link
Collaborator

sug0 commented Nov 11, 2025

oh one thing this might "break" is ctrl+c after running the implicit shielded sync, because it installs a SIGINT handler that overrides the default handler. if an rpc is hanging, we might not be able to ctrl+c out of namadac

@grarco
Copy link
Collaborator Author

grarco commented Nov 13, 2025

oh one thing this might "break" is ctrl+c after running the implicit shielded sync, because it installs a SIGINT handler that overrides the default handler. if an rpc is hanging, we might not be able to ctrl+c out of namadac

Right, I think we could fix this in two ways:

  1. We could launch the implicit shielded-sync function in a child process and wait on it. The custom handler for SIGINT should be relative to the child only and rpcs shouldn't block the submit_* functions anymore
  2. We just uninstall the custom handler after we are done with the sync process

I've added the do-not-merge label for now until we have a fix for this

@grarco grarco added the do-not-merge Do not merge for now label Nov 13, 2025
@codecov
Copy link

codecov bot commented Nov 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@sug0
Copy link
Collaborator

sug0 commented Nov 13, 2025

@grarco I kinda like the first option, despite it being a bit hacky; from namada_apps_lib, we can assume we are executing one of our binaries, so the first value of std::env::args() will give you the executable path (either namada or namadac), from which we should be able to invoke shielded-sync

@grarco grarco force-pushed the grarco/delete-speculative-context branch from 9116705 to d72523d Compare December 6, 2025 17:12
@grarco
Copy link
Collaborator Author

grarco commented Dec 6, 2025

@sug0 attempted the change in d72523d

@grarco grarco force-pushed the grarco/delete-speculative-context branch from d72523d to b14deef Compare December 8, 2025 17:30
@grarco
Copy link
Collaborator Author

grarco commented Dec 8, 2025

@sug0 attempted the change in d72523d

Never mind, the previous solution with the child process was giving me problems when it came to tests. I've now tried to override the custom signal handlers after we are done with the shielded-sync command. It might be slightly hacky but it's easier and faster to implement

@grarco grarco removed the do-not-merge Do not merge for now label Dec 8, 2025
@grarco grarco requested a review from sug0 December 8, 2025 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking:api public API breaking change breaking:cli command line breaking change breaking:SDK SDK breaking change cli client MASP SDK UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Evaluate removal of the speculative shielded context

3 participants