feat(solana-swap): Implement basic maker functionality for SOL swaps by r2st · Pull Request #1 · GLEECBTC/satomic-swap

r2st · 2024-03-28T06:55:35Z

No description provided.

laruh

Thanks a lot for this feature!

I have some notes and questions.

PS: please commit Cargo.lock file, so security team (and devs actually) can see the whole deps tree. and it helps to be sure that all contributors have same deps.

src/etomic_swap.rs

Cargo.toml

src/tests.rs

src/etomic_swap.rs

src/tests.rs

src/etomic_swap.rs

README.md

src/tests.rs

shamardy

Thanks for the PR! First review iteration where the purpose is to make the code more readable to better review it in subsequent iterations.

src/satomic_swap.rs

laruh

Thanks for the fixes, next iteration

src/satomic_swap.rs

laruh · 2024-04-03T14:27:56Z

src/satomic_swap.rs

+            {
+                let create_instruction = system_instruction::create_account(
+                    sender_account.key,
+                    vault_pda_data.key,
+                    rent_exemption_lamports,
+                    41,
+                    program_id,
+                );
+
+                let account_infos = vec![
+                    sender_account.clone(),
+                    vault_pda_data.clone(),
+                    system_program_account.clone(),
+                ];
+
+                invoke_signed(&create_instruction, &account_infos, &[vault_seeds_data])?;
+
+                let data = &mut vault_pda_data.try_borrow_mut_data()?;
+                if data.len() < payment_bytes.len() {
+                    msg!("Error: Account data buffer too small");
+                    return Err(ProgramError::AccountDataTooSmall);
+                }
+
+                data[..payment_bytes.len()].copy_from_slice(&payment_bytes);
+            }


Why do you need this inner scope? I dont have compilation errors moving this inner scope operations to the outer.

To improve readability and maintainability please break the code into smaller well named functions, like Omer suggested.

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

…e transfer operation. Only sender and receiver are involved. Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

* feat: Solana Swap with all of previous PR suggestions applied. * feat: Providing system_program_account should not be necessary for the transfer operation. Only sender and receiver are involved. * feat: Max line width fixed with cargo fmt

shamardy · 2024-06-06T12:36:44Z

The latest commit does the following as stated by @mj-blockydevs

List of applied suggestions:

Removed msg calls.

Removed unused variables. IMPORTANT: For now, I've left unused struct properties to avoid changing the communication interface with the program. My main goal was to apply all suggestions without touching the "tests.rs" file, to ensure the business logic behind the program remained unchanged.

Broke the code into smaller functions. I've not only moved them to separate functions but also placed the functions in a dedicated module for clarity. Let me know if you are okay with this decision.

Using invoke_signed without the system program account. Let me know if I understood this particular requirement correctly.

Replaced panicking asserts with supported Solana Errors. Let me know if the ones I used are okay for you.

Removed inner scopes where they were not necessary.

Do you want me to prepare unit tests for the functions moved to the /utils directory, or are the already prepared tests sufficient for us at this stage?

@mj-blockydevs @laruh let's continue the review here since @mj-blockydevs now has access to the repo.

shamardy · 2024-06-06T12:51:42Z

Removed unused variables. IMPORTANT: For now, I've left unused struct properties to avoid changing the communication interface with the program. My main goal was to apply all suggestions without touching the "tests.rs" file, to ensure the business logic behind the program remained unchanged.

Will cover it in next review if there are any problems.

I've not only moved them to separate functions but also placed the functions in a dedicated module for clarity. Let me know if you are okay with this decision.

Code looks much better this way, thanks for this refactor.

Using invoke_signed without the system program account. Let me know if I understood this particular requirement correctly.

Looks good on first look, will check it extensively in the next review iteration.

Replaced panicking asserts with supported Solana Errors. Let me know if the ones I used are okay for you.

Will check them case by case in the review.

Do you want me to prepare unit tests for the functions moved to the /utils directory, or are the already prepared tests sufficient for us at this stage?

Unit tests for each module would be great to have but not necessary at this stage, it's up to you if you want to add them if there are no other higher priority items to finish first.

shamardy

Thanks you for the fixes! Next review iteration!

Cargo.toml

README.md

shamardy · 2024-06-06T13:48:41Z

src/instruction.rs

+}
+
+impl AtomicSwapInstruction {
+    pub fn unpack(


We should look into using borsh crate for serialization and deserialization instead of pack/unpack, maybe it will provide better performance but if not and the performance difference is negligible, using borsh will lead to less errors. This is not required in this PR but we should look into it in the future when completing the swap program for v1 and v2 swaps.

shamardy · 2024-06-06T13:53:04Z

src/satomic_swap.rs

+                vault_seeds_data,
+                payment_bytes,
+            )?;
+


I guess we need to transfer the tokens here.

Yes, I'll leave comments in the code to remind us about it. However, it is out of scope for now. We will address this issue in a separate PR.

shamardy · 2024-06-06T13:54:55Z

src/satomic_swap.rs

+            if token_program != Pubkey::new_from_array([0; 32]) {
+                return Err(ProgramError::Custom(NOT_SUPPORTED));
+            }
+            transfer_lamports(vault_pda, receiver_account, vault_seeds, amount)?;


We need also to add the case of tokens transfer here

shamardy · 2024-06-06T13:55:08Z

src/satomic_swap.rs

+            if token_program != Pubkey::new_from_array([0; 32]) {
+                return Err(ProgramError::Custom(NOT_SUPPORTED));
+            }
+            transfer_lamports(vault_pda, sender_account, vault_seeds, amount)?;


laruh

Great, here are notes from my side

laruh · 2024-06-07T04:17:45Z

src/utils/payment.rs

+use crate::error_code::{INVALID_PAYMENT_HASH, INVALID_PAYMENT_STATE, SWAP_ACCOUNT_NOT_FOUND};
+use crate::payment::{Payment, PaymentState};
+use solana_program::{
+    account_info::AccountInfo, entrypoint::ProgramResult, program::invoke_signed,
+    program_error::ProgramError, system_instruction,
+};


I suppose there is a lack of spl_token crate usage to provide transfer_spl_tokens implementation

Yes, it will be necessary to support token transfers. This will be implemented later.

Cargo.toml

src/utils/hash.rs

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

shamardy

Thanks a lot for the fixes! A few non-blocker comments that should be fixed here before merge.
@laruh can you please check that all your comments are fixed and that you have no more comments.

Cargo.toml

shamardy · 2024-06-13T10:51:02Z

src/instruction.rs

AtomicSwapInstruction/pack/unpack and the error codes are duplicated/used here GLEECBTC/komodo-defi-framework#2091, I suggest removing them from #2091 once this PR is merged and import them from this repo/crate. Then we would need to edit them only in one place in the future if the need arises.

src/utils/account.rs

laruh

Thanks for the fixes, init v1 LGTM!

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

feat(solana-swap): initial version v1

9139933

r2st changed the base branch from main to dev March 28, 2024 06:59

lint fix

d69dc6b

shamardy requested review from laruh and shamardy March 28, 2024 20:52

laruh suggested changes Mar 29, 2024

View reviewed changes

r2st added 2 commits April 1, 2024 12:34

resolved issues @laruh

28da942

resolved issues @laruh

b5f9477

shamardy reviewed Apr 2, 2024

View reviewed changes

src/satomic_swap.rs Outdated Show resolved Hide resolved

src/satomic_swap.rs Outdated Show resolved Hide resolved

src/satomic_swap.rs Outdated Show resolved Hide resolved

src/satomic_swap.rs Outdated Show resolved Hide resolved

laruh suggested changes Apr 3, 2024

View reviewed changes

mj-blockydevs and others added 4 commits June 5, 2024 16:39

feat: Solana Swap with all of previous PR suggestions applied.

94ad5a6

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

feat: Providing system_program_account should not be necessary for th…

99373ec

…e transfer operation. Only sender and receiver are involved. Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

feat: Max line width fixed with cargo fmt

a989151

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

shamardy added the under review label Jun 6, 2024

shamardy reviewed Jun 6, 2024

View reviewed changes

shamardy requested a review from laruh June 6, 2024 13:55

laruh suggested changes Jun 7, 2024

View reviewed changes

mj-blockydevs added 4 commits June 12, 2024 16:19

feat(solana-swap-v1): solana swap v1

b2936be

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

feat(solana-swap-v1): readme updated

4011cc2

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

feat(solana-swap-v1): optimizing release and build profiles

cc59c01

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

feat(solana-swap-v1): optimizing release and build profiles

338d053

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

shamardy approved these changes Jun 13, 2024

View reviewed changes

laruh approved these changes Jun 13, 2024

View reviewed changes

shamardy mentioned this pull request Jun 13, 2024

feat(solana-swap): solana swap protocol v1 POC GLEECBTC/komodo-defi-framework#2091

Merged

mj-blockydevs added 3 commits June 13, 2024 18:42

feat(solana-swap-v1): additional changes in cargo toml after pr

292b1e5

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

feat(solana-swap-v1): moving account size to a const

8410262

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

feat(solana-swap-v1): more universal name to fit parents context

e3eb883

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

feat(solana-swap-v1): add the reason for late validation

9d8628f

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas+instead@blockydevs.com>

shamardy approved these changes Jun 14, 2024

View reviewed changes

shamardy changed the title ~~feat(solana-swap): initial version v1~~ feat(solana-swap): Implement basic maker functionality for SOL swaps Jun 14, 2024

shamardy merged commit acad67f into dev Jun 14, 2024

shamardy deleted the solana-swap-v1 branch June 14, 2024 12:49

Conversation

r2st commented Mar 28, 2024

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shamardy commented Jun 6, 2024

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

laruh 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!

Uh oh!

Uh oh!

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shamardy commented Jun 6, 2024 •

edited

Loading