fix: use SPL ZRC20 withdraw fee to cover the ATA account creation rent fee #67
Conversation
assuming the withdrawal already paid at least this much in ZRC20 withdraw fee.
brewmaster012
requested review from
lumtis,
ws4charlie and
skosito
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
========================================
- Coverage 6.32% 5.85% -0.48%
========================================
Files 1 1
Lines 411 410 -1
========================================
- Hits 26 24 -2
- Misses 385 386 +1 ☔ View full report in Codecov by Sentry. |
| @@ -386,6 +386,9 @@ pub mod gateway { | |||
| Errors::SPLAtaAndMintAddressMismatch, | |||
| ); | |||
|
|
|||
| let cost_gas = 5000; // default gas cost in lamports | |||
| let cost_ata_create = &mut 2039280; // default SPL ATA account creation rent fee in lamports | |||
There was a problem hiding this comment.
Is it an universal constant? Any link we can provide how the value is determined? I would use a ceiling value to take some more security like 2040000 or 2100000
There was a problem hiding this comment.
i think it could be something that is calculated based on rent and space that ATA takes, but lets wait for @brewmaster012
this is same value i came up with by some research
There was a problem hiding this comment.
I think this number is only for ZRC20 gas_limit configuration reference. The cost_ata_create gets overwritten right after by *cost_ata_create = bal_before - bal_after; without being read.
I'm not against using rounded number because I myself also used it for Solana rent exampt here
There was a problem hiding this comment.
it should be a constant, but let me see if using a computed is viable.
There was a problem hiding this comment.
we'll use whatever the CPI call actually debits from the signer. No longer need this constant.
There was a problem hiding this comment.
addressed in a92a84f
| @@ -386,6 +386,9 @@ pub mod gateway { | |||
| Errors::SPLAtaAndMintAddressMismatch, | |||
| ); | |||
|
|
|||
| let cost_gas = 5000; // default gas cost in lamports | |||
| let cost_ata_create = &mut 2039280; // default SPL ATA account creation rent fee in lamports | |||
There was a problem hiding this comment.
i think it could be something that is calculated based on rent and space that ATA takes, but lets wait for @brewmaster012
this is same value i came up with by some research
|
|
||
| let rent_payer_info = ctx.accounts.rent_payer_pda.to_account_info(); | ||
| let cost = bal_before - bal_after; | ||
| rent_payer_info.sub_lamports(cost)?; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
actually this might not make much sense, let me remove it
There was a problem hiding this comment.
removed rent payer PDA in 4617aa4
| @@ -452,6 +447,12 @@ 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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
addressed in a92a84f
|
|
||
| const to = await spl.getAssociatedTokenAddress(mint.publicKey, wallet2.publicKey); | ||
|
|
||
| let to_ata_bal = await conn.getBalance(to); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
don't think a created account can have 0 lamports--it needs minimum rent
| @@ -452,6 +447,12 @@ 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 | |||
| // otherwise the previous CPI calls might fail with error: | |||
There was a problem hiding this comment.
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
closes #63
closes #4
conflicts with PR #65 : should only consider merging one.
important: all ZRC20 of SPL tokens must charge at least 2039280 lamports (0.002039280 SOL) as withdraw fee, otherwise the program PDA account might run into deficit.