namada icon indicating copy to clipboard operation
namada copied to clipboard

Remove speculative context

Open grarco opened this issue 2 months ago • 7 comments

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
  • [x] If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
    • Relevant PR if applies: https://github.com/namada-net/namada-docs/pull/459
  • [ ] 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 avatar Nov 03 '25 15:11 grarco

🧪 CI Insights

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

🟢 All jobs passed!

But CI Insights is watching 👀

mergify[bot] avatar Nov 03 '25 16:11 mergify[bot]

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

sug0 avatar Nov 11 '25 18:11 sug0

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 avatar Nov 13 '25 09:11 grarco

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests.

:loudspeaker: Thoughts on this report? Let us know!

codecov[bot] avatar Nov 13 '25 09:11 codecov[bot]

@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

sug0 avatar Nov 13 '25 12:11 sug0

@sug0 attempted the change in d72523dc85bdb5f1c44cf2e660b29483d8027d6e

grarco avatar Dec 06 '25 17:12 grarco

@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 avatar Dec 08 '25 19:12 grarco