refactor deposit_and_call and deposit_spl_token_and_call
#35
Conversation
so that it matches deposit instruction.
brewmaster012
requested review from
lumtis,
fbac and
ws4charlie
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
========================================
+ Coverage 8.43% 8.60% +0.17%
========================================
Files 1 1
Lines 249 244 -5
========================================
Hits 21 21
+ Misses 228 223 -5 ☔ View full report in Codecov by Sentry. |
brewmaster012
force-pushed
the
deposit-spl-and-call
branch
2 times, most recently
from
6316158
to
1c8c70c
Compare
brewmaster012
force-pushed
the
deposit-spl-and-call
branch
from
1c8c70c
to
0e18af4
Compare
| message, | ||
| ); | ||
|
|
||
| deposit(ctx, amount, receiver)?; | ||
| Ok(()) |
There was a problem hiding this comment.
do we need this Ok(()) here since there is one in deposit, probably redundant here
There was a problem hiding this comment.
also why some methods have #[allow(unused)]?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| receiver: [u8; 20], | ||
| message: Vec<u8>, | ||
| ) -> Result<()> { | ||
| require!(message.len() <= 512, Errors::MemoLengthExceeded); |
There was a problem hiding this comment.
do we want old memo.len() >= 20 condition as well? if not and MemoLengthTooShort is not used anymore we can remove it
There was a problem hiding this comment.
also is this message length check something we need on GatewayEVM as well, to be compatible or is it solana specific?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| Ok(()) | ||
| } | ||
|
|
||
| // require tss address signature on the internal message defined in the following |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
good idea. let me try something.
| // 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); |
There was a problem hiding this comment.
Should we check also for message.len too short?
There was a problem hiding this comment.
there is comment above saying it can be smaller than 20, but maybe we can check for 0 value
There was a problem hiding this comment.
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.
| message, | ||
| ); | ||
|
|
||
| deposit(ctx, amount, receiver)?; |
There was a problem hiding this comment.
How/where do we emit the message so zEVM receives it?
There was a problem hiding this comment.
In the function parameter
|
going to merge this PR, and do more regimented documentation in another PR. |
refactor deposit spl token into two functions, one just deposit, and the other deposit and call.
Refactor
deposit_and_callanddeposit_spl_token_and_callto reuse thedepositanddeposit_spl_token.added some comments on the instruction processors.