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

Jump to bottom

refactor deposit_and_call and deposit_spl_token_and_call #35

Merged
merged 8 commits into from
Oct 3, 2024
Merged

refactor deposit_and_call and deposit_spl_token_and_call #35

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5abfbaf
remove memo from deposit_spl_token
brewmaster012 Oct 2, 2024
6f4eccd
refactor deposit*_and_call instructions to reuse deposit*
brewmaster012 Oct 2, 2024
acbd25c
format
brewmaster012 Oct 2, 2024
b0318bb
update comment in code that is inaccurate
brewmaster012 Oct 2, 2024
0e18af4
address some CI complaints
brewmaster012 Oct 2, 2024
f352dc3
add some light comments to instructions
brewmaster012 Oct 2, 2024
f9151ac
silence intentional unused variable warnings
brewmaster012 Oct 2, 2024
0d81e93
add some test
brewmaster012 Oct 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
68 changes: 42 additions & 26 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 @@ -48,6 +48,7 @@ pub mod gateway {
Ok(())
}

// admin function to pause or unpause deposit
pub fn set_deposit_paused(ctx: Context<UpdatePaused>, deposit_paused: bool) -> Result<()> {
let pda = &mut ctx.accounts.pda;
require!(
Expand Down Expand Up @@ -84,7 +85,15 @@ pub mod gateway {
Ok(())
}

pub fn deposit(ctx: Context<Deposit>, amount: u64, receiver: [u8; 20]) -> Result<()> {
// 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
// receiver: ethereum address (20Bytes) of the receiver on ZetaChain zEVM
pub fn deposit(
ctx: Context<Deposit>,
amount: u64,
receiver: [u8; 20], // not used in this program; for directing zetachain protocol only
) -> Result<()> {
let pda = &mut ctx.accounts.pda;
require!(!pda.deposit_paused, Errors::DepositPaused);

Expand All @@ -106,43 +115,31 @@ pub mod gateway {
Ok(())
}

// deposit SOL into this program and the `receiver` on ZetaChain zEVM
// will get corresponding ZRC20 credit. The `receiver` should be a contract
// on zEVM and the `message` will be used as input data for the contract call.
// The `receiver` contract on zEVM will get the SOL ZRC20 credit and receive the `message`.
pub fn deposit_and_call(
ctx: Context<Deposit>,
amount: u64,
receiver: [u8; 20],
message: Vec<u8>,
) -> Result<()> {
require!(message.len() <= 512, Errors::MemoLengthExceeded);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check also for message.len too short?

Copy link
Contributor

Choose a reason for hiding this comment

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

there is comment above saying it can be smaller than 20, but maybe we can check for 0 value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 size message seems to be no problem; the app contract can expect empty message; maybe there is use case for that. Let's not be too pedantic here.


let pda = &mut ctx.accounts.pda;
require!(!pda.deposit_paused, Errors::DepositPaused);

let cpi_context = CpiContext::new(
ctx.accounts.system_program.to_account_info(),
system_program::Transfer {
from: ctx.accounts.signer.to_account_info().clone(),
to: ctx.accounts.pda.to_account_info().clone(),
},
);
system_program::transfer(cpi_context, amount)?;
msg!(
"{:?} deposits {:?} lamports to PDA and call contract {:?} with message {:?}",
ctx.accounts.signer.key(),
amount,
receiver,
message,
);

deposit(ctx, amount, receiver)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

How/where do we emit the message so zEVM receives 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.

In the function parameter

Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this Ok(()) here since there is one in deposit, probably redundant here

Copy link
Contributor

Choose a reason for hiding this comment

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

also why some methods have #[allow(unused)]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we want to get rid of the Ok(()) at the end, it would look something like:

        Ok(deposit_spl_token(ctx, amount, receiver)?)

at the end; not sure i like this better than with the Ok(()) at the end; natuarally all instructions should have Ok(()) at the end.

Copy link
Contributor Author

@brewmaster012 brewmaster012 Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

for the #[allow(unused)], it's for the unused paramter message or receiver
These two are not useful in this solana program; they are just instructions for zetachain cctx processing.
As Solana does not really have typed event, we'll just use instruction parameter instead. At least they are typed.
I don't want to precede them with _message, _receiver, hence #[allow(unused)] for silencing warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, thanks for info
regarding Ok, i thought the one in deposit will finish instruction and return to caller and this one would never be reached, but i dont know if that is true when one instruction is called from another, its ok to keep it here anyways

}

// deposit SPL token into this program and the `receiver` on ZetaChain zEVM
// will get corresponding ZRC20 credit.
// amount: amount of SPL token to deposit
// receiver: ethereum address (20Bytes) of the receiver on ZetaChain zEVM
#[allow(unused)]
pub fn deposit_spl_token(
ctx: Context<DepositSplToken>,
amount: u64,
memo: Vec<u8>,
receiver: [u8; 20], // unused in this program; for directing zetachain protocol only
) -> Result<()> {
require!(memo.len() >= 20, Errors::MemoLengthTooShort);
require!(memo.len() <= 512, Errors::MemoLengthExceeded);
let token = &ctx.accounts.token_program;
let from = &ctx.accounts.from;

Expand Down Expand Up @@ -174,7 +171,25 @@ pub mod gateway {
Ok(())
}

// only tss address stored in PDA can call this instruction
// like `deposit_spl_token` instruction,
// deposit SPL token into this program and the `receiver` on ZetaChain zEVM
// will get corresponding ZRC20 credit. The `receiver` should be a contract
// on zEVM and the `message` will be used as input data for the contract call.
// The `receiver` contract on zEVM will get the SPL token ZRC20 credit and receive the `message`.
#[allow(unused)]
pub fn deposit_spl_token_and_call(
ctx: Context<DepositSplToken>,
amount: u64,
receiver: [u8; 20],
message: Vec<u8>,
) -> Result<()> {
require!(message.len() <= 512, Errors::MemoLengthExceeded);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want old memo.len() >= 20 condition as well? if not and MemoLengthTooShort is not used anymore we can remove it

Copy link
Contributor

Choose a reason for hiding this comment

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

also is this message length check something we need on GatewayEVM as well, to be compatible or is it solana specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

message could be less than 20 bytes. I guess we could remove the error MemoLengthTooShort

There is a restriction on size of Solana tx (1023B??). Might as well have a limit in the instruction.
Not sure we want to have the same limit across all networks though.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, yes limit is 1232
we can maybe write a test to break this condition and test upper bound edge case, to be sure if limit can be this number

deposit_spl_token(ctx, amount, receiver)?;
Ok(())
}

// require tss address signature on the internal message defined in the following
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we can add issue to write comments in same format for all functions, something like

// description...
// param1 explanation
...
// paramN explanation

in solidity there is natspec, but couldnt find same for solana contracts, but we can make some format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea. let me try something.

// concatenated_buffer vec.
pub fn withdraw(
ctx: Context<Withdraw>,
amount: u64,
Expand Down Expand Up @@ -216,7 +231,8 @@ pub mod gateway {
Ok(())
}

// only tss address stored in PDA can call this instruction
// require tss address signature on the internal message defined in the following
// concatenated_buffer vec.
pub fn withdraw_spl_token(
ctx: Context<WithdrawSPLToken>,
amount: u64,
Expand Down
4 changes: 2 additions & 2 deletions tests/protocol-contracts-solana.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ describe("some tests", () => {
);
tx.add(memoInst);
const depositInst = await gatewayProgram.methods.depositSplToken(
new anchor.BN(1_000_000), address).accounts(
new anchor.BN(1_000_000), Array.from(address)).accounts(
{
from: tokenAccount.address,
to: pda_ata.address,
Expand All @@ -157,7 +157,7 @@ describe("some tests", () => {


try {
await gatewayProgram.methods.depositSplToken(new anchor.BN(1_000_000), address).accounts(
await gatewayProgram.methods.depositSplToken(new anchor.BN(1_000_000), Array.from(address)).accounts(
{
from: tokenAccount.address,
to: wallet_ata,
Expand Down
Loading