Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use SPL ZRC20 withdraw fee to cover the ATA account creation rent fee #67

Merged
merged 7 commits into from
Dec 3, 2024
+39 −77
Merged

fix: use SPL ZRC20 withdraw fee to cover the ATA account creation rent fee #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9118d4c
have PDA reimburse the ATA creation fee;
brewmaster012 Dec 3, 2024
6aaadc0
fmt
brewmaster012 Dec 3, 2024
a92a84f
adjust rent reimbursement
brewmaster012 Dec 3, 2024
4617aa4
remove rent payer PDA
brewmaster012 Dec 3, 2024
9489907
cleanup test
brewmaster012 Dec 3, 2024
ed9ea15
cleanup test2
brewmaster012 Dec 3, 2024
505b74b
updated comment
brewmaster012 Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 16 additions & 31 deletions programs/protocol-contracts-solana/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,10 +167,6 @@ pub mod gateway {
Ok(())
}

pub fn initialize_rent_payer(_ctx: Context<InitializeRentPayer>) -> Result<()> {
Ok(())
}

// deposit SOL into this program and the `receiver` on ZetaChain zEVM
// will get corresponding ZRC20 credit.
// amount: amount of lamports (10^-9 SOL) to deposit
Expand Down Expand Up @@ -386,6 +382,9 @@ pub mod gateway {
Errors::SPLAtaAndMintAddressMismatch,
);

let cost_gas = 5000; // default gas cost in lamports
let cost_ata_create = &mut 0; // will be updated if ATA creation is needed

// test whether the recipient_ata is created or not; if not, create it
let recipient_ata_account = ctx.accounts.recipient_ata.to_account_info();
if recipient_ata_account.lamports() == 0
Expand All @@ -397,8 +396,7 @@ pub mod gateway {
recipient_ata_account.key(),
ctx.accounts.recipient.key(),
);
let signer_info = &ctx.accounts.signer.to_account_info();
let bal_before = signer_info.lamports();
let bal_before = ctx.accounts.signer.lamports();
invoke(
&create_associated_token_account(
ctx.accounts.signer.to_account_info().key,
Expand All @@ -419,22 +417,15 @@ pub mod gateway {
.clone(),
],
)?;
let bal_after = signer_info.lamports();
let bal_after = ctx.accounts.signer.lamports();
*cost_ata_create = bal_before - bal_after;

msg!("Associated token account for recipient created!");
msg!(
"Refunding the rent paid by the signer {:?}",
"Refunding the rent ({:?} lamports) paid by the signer {:?}",
cost_ata_create,
brewmaster012 marked this conversation as resolved.
Show resolved Hide resolved
ctx.accounts.signer.to_account_info().key
);

let rent_payer_info = ctx.accounts.rent_payer_pda.to_account_info();
let cost = bal_before - bal_after;
rent_payer_info.sub_lamports(cost)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove rent_payer from program completely? it would be one less account in withdraw_spl (would be small change on zetaclient side too) and also removing function to init rent payer

anyways this will need update in e2e test covering this which we have on node atm, but it will be small change, i can take care of it when we merge this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but i thought about it, we might have a need to store withdraw fee excess in a segregated account; this rent_payer_pda might be it.

this helps distributing fee excess to say zeta delegators easier in the future, so i say we keep it but not initialize it yet.

Copy link
Contributor Author

@brewmaster012 brewmaster012 Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

actually this might not make much sense, let me remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed rent payer PDA in 4617aa4

signer_info.add_lamports(cost)?;
msg!(
"Signer refunded the ATA account creation rent amount {:?} lamports",
cost,
);
}

let xfer_ctx = CpiContext::new_with_signer(
Expand All @@ -452,6 +443,14 @@ pub mod gateway {

transfer_checked(xfer_ctx, amount, decimals)?;
msg!("withdraw spl token successfully");
// Note: this pda.sub_lamports() must be done here due to this issue https://github.com/solana-labs/solana/issues/9711
Copy link
Contributor

Choose a reason for hiding this comment

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

in case ata already exists, this looks redundant, maybe we can have flag in if condition that checks for ata existance and based on that we do refunding here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in a92a84f

// otherwise the previous CPI calls might fail with error:
Copy link
Contributor

Choose a reason for hiding this comment

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

if we can add more details on how we guarantee that there always will be funds in pda and no way to drain it that would be great, since it's not obvious from the program only

// "sum of account balances before and after instruction do not match"
// Note2: to keep PDA from deficit, all SPL ZRC20 contracts needs to charge withdraw fee of
// at least 5000(gas)+2039280(rent) lamports.
let reimbursement = cost_gas + *cost_ata_create;
pda.sub_lamports(reimbursement)?;
ctx.accounts.signer.add_lamports(reimbursement)?;

Ok(())
}
Expand Down Expand Up @@ -589,9 +588,6 @@ pub struct WithdrawSPLToken<'info> {
#[account(mut)]
pub recipient_ata: AccountInfo<'info>,

#[account(mut, seeds = [b"rent-payer"], bump)]
pub rent_payer_pda: Account<'info, RentPayerPda>,

pub token_program: Program<'info, Token>,

pub associated_token_program: Program<'info, AssociatedToken>,
Expand Down Expand Up @@ -675,17 +671,6 @@ pub struct Unwhitelist<'info> {
pub system_program: Program<'info, System>,
}

#[derive(Accounts)]
pub struct InitializeRentPayer<'info> {
#[account(init, payer = authority, space = 8, seeds = [b"rent-payer"], bump)]
pub rent_payer_pda: Account<'info, RentPayerPda>,

#[account(mut)]
pub authority: Signer<'info>,

pub system_program: Program<'info, System>,
}

#[account]
pub struct Pda {
nonce: u64, // ensure that each signature can only be used once
Expand Down
69 changes: 23 additions & 46 deletions tests/protocol-contracts-solana.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,6 @@ describe("Gateway", () => {
gatewayProgram.programId,
);

let rentPayerSeeds = [Buffer.from("rent-payer", "utf-8")];
let [rentPayerPdaAccount] = anchor.web3.PublicKey.findProgramAddressSync(
rentPayerSeeds,
gatewayProgram.programId,
);

it("Initializes the program", async () => {
await gatewayProgram.methods.initialize(tssAddress, chain_id_bn).rpc();

Expand All @@ -143,18 +137,6 @@ describe("Gateway", () => {
expect(err).to.be.not.null;
}
});
it("initialize the rent payer PDA",async() => {
await gatewayProgram.methods.initializeRentPayer().rpc();
let instr = web3.SystemProgram.transfer({
fromPubkey: wallet.publicKey,
toPubkey: rentPayerPdaAccount,
lamports: 100000000,
});
let tx = new web3.Transaction();
tx.add(instr);
await web3.sendAndConfirmTransaction(conn,tx,[wallet]);
});


it("Mint a SPL USDC token", async () => {
// now deploying a fake USDC SPL Token
Expand Down Expand Up @@ -343,33 +325,6 @@ describe("Gateway", () => {

});

it("withdraw SPL token to a non-existent account should succeed by creating it", async () => {
let seeds = [Buffer.from("rent-payer", "utf-8")];
const [rentPayerPda] = anchor.web3.PublicKey.findProgramAddressSync(
seeds,
gatewayProgram.programId,
);
let rentPayerPdaBal0 = await conn.getBalance(rentPayerPda);
let pda_ata = await spl.getAssociatedTokenAddress(mint.publicKey, pdaAccount, true);
const pdaAccountData = await gatewayProgram.account.pda.fetch(pdaAccount);
const hexAddr = bufferToHex(Buffer.from(pdaAccountData.tssAddress));
const amount = new anchor.BN(500_000);
const nonce = pdaAccountData.nonce;
const wallet2 = anchor.web3.Keypair.generate();

const to = await spl.getAssociatedTokenAddress(mint.publicKey, wallet2.publicKey);

let to_ata_bal = await conn.getBalance(to);
expect(to_ata_bal).to.be.eq(0); // the new ata account (owned by wallet2) should be non-existent;
const txsig = await withdrawSplToken(mint, usdcDecimals, amount, nonce, pda_ata, to, wallet2.publicKey, keyPair, gatewayProgram);
to_ata_bal = await conn.getBalance(to);
expect(to_ata_bal).to.be.gt(2_000_000); // the new ata account (owned by wallet2) should be created

// rent_payer_pda should have reduced balance

let rentPayerPdaBal1 = await conn.getBalance(rentPayerPda);
expect(rentPayerPdaBal0-rentPayerPdaBal1).to.be.eq(to_ata_bal); // rentPayer pays rent
});

it("fails to deposit if receiver is empty address", async() => {
try {
Expand Down Expand Up @@ -415,7 +370,29 @@ describe("Gateway", () => {
let bal3 = await conn.getBalance(to);
expect(bal3).to.be.gte(500_000_000);
})


it("withdraw SPL token to a non-existent account should succeed by creating it", async () => {
let rentPayerPdaBal0 = await conn.getBalance(pdaAccount);
let pda_ata = await spl.getAssociatedTokenAddress(mint.publicKey, pdaAccount, true);
const pdaAccountData = await gatewayProgram.account.pda.fetch(pdaAccount);
const amount = new anchor.BN(500_000);
const nonce = pdaAccountData.nonce;
const wallet2 = anchor.web3.Keypair.generate();
const to = await spl.getAssociatedTokenAddress(mint.publicKey, wallet2.publicKey);

let to_ata_bal = await conn.getBalance(to);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there is no way to check that ata doesnt exist? it still might exist with 0 balance so test might be false positive here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't think a created account can have 0 lamports--it needs minimum rent

expect(to_ata_bal).to.be.eq(0); // the new ata account (owned by wallet2) should be non-existent;
const txsig = await withdrawSplToken(mint, usdcDecimals, amount, nonce, pda_ata, to, wallet2.publicKey, keyPair, gatewayProgram);
to_ata_bal = await conn.getBalance(to);
expect(to_ata_bal).to.be.gt(2_000_000); // the new ata account (owned by wallet2) should be created

// pda should have reduced balance
let rentPayerPdaBal1 = await conn.getBalance(pdaAccount);
// expected reimbursement to be gas fee (5000 lamports) + ATA creation cost 2039280 lamports
expect(rentPayerPdaBal0-rentPayerPdaBal1).to.be.eq(to_ata_bal + 5000); // rentPayer pays rent
});


it("fails to deposit and call if receiver is empty address", async() => {
try {
await gatewayProgram.methods.depositAndCall(new anchor.BN(1_000_000_000), Array(20).fill(0), Buffer.from("hello", "utf-8")).accounts({}).rpc();
Expand Down
Loading