fix: fixed undelegation of ephemeral balance #76

taco-paco · 2025-05-06T04:52:28Z

⚠️ 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	Bug	Yes	Link

Problem

Ephemeral balance was undelegated to delegation program itslef. Expected behavior delegation to system program.

Solution

Introduced new method undelegate_ephemeral_balance that will call undelegate and then reassign ownership to system_program

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)

Added v1 for CloseEphemeralBalance. Reason is that this change shall be backwards-compatible. For exmaple, some users may have accounts undelegated using old logic, their lamports will be stuck if we remove old CloseEphemeralBalance.

We keep only one version of CloseEphemeralBalance in InstructionBuilder utilities in order to enforce a newer logic. Clients may still recreate old instruction and call "v0" of CloseEphemeralBalance.

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

Greptile Summary

This PR fixes the undelegation of ephemeral balances by ensuring ownership is properly transferred to the system program instead of the delegation program.

Added new src/instruction_builder/undelegate_ephemeral_balance.rs to handle proper ownership reassignment to system program after undelegation
Added CloseEphemeralBalanceV1 discriminator (0x10) while maintaining backward compatibility with v0 for existing accounts
Added comprehensive test coverage in tests/test_top_up.rs to verify undelegation and closing functionality
Modified src/processor/close_ephemeral_balance_v1.rs to properly verify system program ownership and handle lamport transfers
Updated instruction builders to enforce new undelegation logic while allowing clients to still access v0 if needed

The changes look well-structured with proper error handling and test coverage. The backward compatibility approach for CloseEphemeralBalance is appropriate to prevent breaking existing accounts.

_{💡 (5/5) You can turn off certain types of comments like style here!}

…em program. Added new version of close_ephemeral_balance to support it. kept old one for backward-compatability

greptile-apps

_{10 file(s) reviewed, 5 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

src/instruction_builder/close_ephemeral_balance.rs

src/instruction_builder/undelegate_ephemeral_balance.rs

tests/test_top_up.rs

Dodecahedr0x

LGTM

Dodecahedr0x · 2025-05-06T07:44:44Z

src/discriminator.rs

@@ -45,7 +49,8 @@ impl DlpDiscriminator {
 impl TryFrom<[u8; 8]> for DlpDiscriminator {
    type Error = ProgramError;
    fn try_from(bytes: [u8; 8]) -> Result<Self, Self::Error> {
-        match bytes[0] {
+        let discriminator = u64::from_le_bytes(bytes);


Isn't this more CU-intense without benefits (until the program reaches 255 instructions)?

Dodecahedr0x · 2025-05-06T08:11:09Z

src/instruction_builder/close_ephemeral_balance.rs


 use crate::discriminator::DlpDiscriminator;
 use crate::pda::ephemeral_balance_pda_from_payer;

 /// Creates instruction to close an ephemeral balance account
-/// See [crate::processor::process_close_ephemeral_balance] for docs.
+/// See [crate::processor::process_close_ephemeral_balance_v1] for docs.
+/// [crate::processor::process_close_ephemeral_balance] now deprecated


Should probably annotate process_close_ephemeral_balance with #[deprecated(since="...", note="...")]

Dodecahedr0x · 2025-05-06T08:14:16Z

src/processor/close_ephemeral_balance_v1.rs

+    if ephemeral_balance_account.owner != &system_program::id() {
+        msg!(
+            "ephemeral balance expected to be owned by system program: {}",
+            system_program::id()


I think it would be more interesting to log ephemeral_balance_account.owner rather than the well-known system program. It might be even better to give a static message to avoid the CU cost of formatting strings

Dodecahedr0x · 2025-05-06T08:17:32Z

src/processor/close_ephemeral_balance_v1.rs

+        return Ok(());
+    }
+
+    let ephemeral_balance_bump_slice: &[u8] = &[ephemeral_balance_bump];


Shouldn't it use close_pda at some point like the old process_close_ephemeral_balance?

As I understand this practically closes account, since it doesn't have lamports and data

Dodecahedr0x · 2025-05-06T08:27:57Z

src/processor/top_up_ephemeral_balance.rs

@@ -56,7 +56,7 @@ pub fn process_top_up_ephemeral_balance(
        create_pda(
            ephemeral_balance_account,
            &system_program::id(),
-            8,
+            0,


I don't understand what causes this change in this PR (and the subsequent changes in tests), I'm guessing it's the core of the fix for something introduced in another PR?

There's 2 points to this

It fixes undelegation. Otherwise we try to call non-existing callback

I reckoned discriminator isn't needed. It is not set anyway. the account just stores lamports, the same as on curve addresses don't have any discriminator. The only thing that @GabrielePicco mentioned is that it might complicate filtering, but the address of this pda shall suffice for filtering

It was there mainly for consistency (discriminator should be set on creation) to allow queries such as getAllEscrowAccounts that are not otherwise possible.

Ah I see now what you meant

GabrielePicco

Unless I'm missing something, I think there is a major problem with this implementation, the ER state is not committed before undelegating. Undelegation should always originate from a CPI executed as part of a transaction in the ER, and should go trough the normal commit/finalize flow, as otherwise the undelegation will revert the original balance

GabrielePicco · 2025-05-06T22:13:50Z

src/instruction_builder/close_ephemeral_balance.rs

        ],
        data: [
-            DlpDiscriminator::CloseEphemeralBalance.to_vec(),
+            DlpDiscriminator::CloseEphemeralBalanceV1.to_vec(),


This is not currently used, so we can avoid the "V1" as it's not gonna be a breaking change

fix: fixed undelegation of ephemeral balance. Now undelegated to syst…

cc8f6a8

…em program. Added new version of close_ephemeral_balance to support it. kept old one for backward-compatability

taco-paco requested review from GabrielePicco and thlorenz May 6, 2025 04:52

greptile-apps bot reviewed May 6, 2025

View reviewed changes

taco-paco added 2 commits May 6, 2025 13:54

refactor: skip transfer if amount is 0

f7e71b4

refactor: addressed greptile comments

d781c42

Dodecahedr0x approved these changes May 6, 2025

View reviewed changes

GabrielePicco requested changes May 6, 2025

View reviewed changes

fix: fixed undelegation of ephemeral balance #76

Are you sure you want to change the base?

fix: fixed undelegation of ephemeral balance #76

Uh oh!

Conversation

taco-paco commented May 6, 2025 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Before & After Screenshots

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

Deploy Notes

Greptile Summary

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Dodecahedr0x left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taco-paco May 7, 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!

GabrielePicco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

taco-paco commented May 6, 2025 •

edited by greptile-apps bot

Loading

taco-paco May 7, 2025 •

edited

Loading