Skip to content

Conversation

@taco-paco
Copy link
Contributor

@taco-paco taco-paco commented Dec 4, 2025

⚠️ NOTE: Use notes like this to emphasize something important about the PR.

This could include other PRs this PR is built on top of; API breaking changes; reasons for why the PR is on hold; or anything else you would like to draw attention to.

Status Type ⚠️ Core Change Issue
Ready Refactor Yes Link

Problem

Right now it is very hard to understand what is the actual error without querying logs and then inspecting them. This is especially critical in Intent execution & recovery.

For example: we have a case, where finalization failed. User schedules new intent with commit of same account, but since finalization failed we will get ProgramError::InvalidAccountOwner error. The problem here is that there're tons of places in Commit instruction that spit out InvalidAccountOwner . So it could be some other thing that went wrong. So in order to know this for certain we would have to use getTransaction to then inspect error, which brings overhead of using network + another error handling cases
"Invalid owner for account. Label: commit state account; account and owner:"
It would be much easier if we would have special code COMMIT_STATE_INVALID_OWNER = 0xsome_code.
This would allow to figure out what went wrong right away from TransactionError type in validator

Solution

Introduce context-aware error mapping for the require_uninitialize_* account/PDA validation helpers.

Each call site now supplies a small context type (e.g. CommitStateAccountCtx, CommitRecordCtx) implementing a RequireUninitializedAccountCtx trait.
This trait maps low-level validation failures (invalid_owner, invalid_seeds, already_initialized, immutable) into specific DlpError codes unique to that account role.

As a result:

Every failure in commit and related instructions now produces a precise, deterministic error code (e.g. CommitStateInvalidAccountOwner) instead of generic ProgramError variants.

Validators and recovery flows can determine the root cause without fetching logs or decoding transaction messages.
Error surfaces become self-describing, improving debugging, intent execution, and protocol safety.

In the future, this pattern can be extended to other helpers (e.g. require_initialized_pda) via additional traits like RequireInitializedCtx.
Having multiple small traits keeps each helper’s contract focused on exactly the errors it can emit, avoids unused methods, and lets instructions opt into finer-grained error codes incrementally without bloating a single “god” interface.

Before & After Screenshots

Insert screenshots of example code output

BEFORE:
[insert screenshot here]

AFTER:
[insert screenshot here]

Other changes (e.g. bug fixes, small refactors)

Deploy Notes

Notes regarding deployment of the contained body of work. These should note any
new dependencies, new scripts, etc.

New scripts:

  • script : script details

New dependencies:

  • dependency : dependency details

Summary by CodeRabbit

  • Refactor

    • Improved error/context handling for commitment, delegation, and undelegation flows to provide clearer diagnostics on validation, ownership, initialization, and immutability issues.
  • Bug Fixes / UX

    • Added more granular, specific error variants so failures report precise causes (invalid seeds, wrong owner, already-initialized, immutable) for affected accounts.

✏️ Tip: You can customize this high-level summary in your review settings.

@magicblock-labs magicblock-labs deleted a comment from coderabbitai bot Dec 10, 2025
Copy link
Contributor

@snawaz snawaz left a comment

Choose a reason for hiding this comment

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

I think it's a good change. 🤔

Previously, we had to mentally correlate two separate log lines:

  • One log showing "commit state account"
  • The next showing InvalidSeeds

We had to combine them in our heads to infer that the commit-state-account had invalid seeds. Hmm. Not great.

However, with the new approach, we directly get CommitStateInvalidSeeds in the logs. No inference needed. Tests can also read this log programmatically with zero ambiguity. That’s definitely nicer. 👍 ✅


The only minor downside, that is not fixed yet, is the verbosity of logs. It was there earlier as well. And since almost all of this information (except the pubkeys) is known at compile time, it feels like we could bundle multiple statically-known pieces into a single log entry ... saving a few CUs and making the log even richer and better. But that’s something for a later PR, assuming Rust’s metaprogramming lets us do it cleanly, as right now I'm not sure if it is even doable.

Copy link
Contributor

@GabrielePicco GabrielePicco left a comment

Choose a reason for hiding this comment

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

LGTM!

@taco-paco taco-paco merged commit 735640a into main Dec 10, 2025
4 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.

4 participants