[message-buffer 13/x] Rust Integration Tests #794

swimricky · 2023-05-02T19:31:40Z

Summary

Add integration tests using solana-program-test crate, add rust-toolchain.toml to pin rust version, add integration tests, update cpi caller auth seeds

vercel · 2023-05-02T19:31:47Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
example-oracle-amm	⬜️ Ignored (Inspect)			May 8, 2023 6:08pm
xc-admin-frontend	⬜️ Ignored (Inspect)			May 8, 2023 6:08pm

swimricky · 2023-05-03T13:22:16Z

@jayantk there's a few tests that are just stubbed out for now but wanted to get this first set checked in since there were a few minor fixes and also to see if there's anything more urgent to work on first

jayantk

I think this is going in the right direction, though i'm having a hard time understanding what's actually being tested in each set of tests because there's a lot of repeated code. Can you please do the inline suggestion to reduce repetition, and then maybe leave a comment on any tests where the logic isn't immediately apparent to the reader? I'll then go back through and think about the test cases in detail.

jayantk · 2023-05-03T14:50:18Z

message_buffer/programs/mock-cpi-caller/src/instructions/add_price.rs

                ctx.accounts.message_buffer_program.key().as_ref(),
-                CPI.as_bytes(),
+                // ctx.accounts.message_buffer_program.key().as_ref(),
+                // UPD_PRICE_WRITE.as_bytes(),


Can you delete this?

jayantk · 2023-05-03T14:51:17Z

message_buffer/programs/mock-cpi-caller/src/instructions/mod.rs


 pub const ACCUMULATOR_UPDATER_IX_NAME: &str = "put_all";
-pub const CPI: &str = "cpi";
+pub const UPD_PRICE_WRITE: &str = "upd_price_write";


is there a reason you changed the PDA derivation path? I think there's no reason that the test needs to use exactly the same derivation path as the production program, right?

jayantk · 2023-05-03T14:53:11Z

message_buffer/programs/mock-cpi-caller/tests/cases/test_create_msg_buffer.rs

+    send_set_allowed_programs(banks_client, payer, &whitelist_pda, &allowed_programs)
+        .await
+        .unwrap();
+    let pyth_price_acct_id = 0u64;


you've copy-pasted a bunch of the lines that set up the tests. This is bad for two reasons: (1) if you ever need to change the code, you have to change it in a lot of places, and (2) it's hard for the reader to understand what's actually being tested in each test.

I suggest making a test harness or something to encapsulate this logic so it isn't repeated. You can take a look at https://github.com/pyth-network/pyth-client/blob/main/program/rust/src/tests/pyth_simulator.rs which is our test harness for the oracle program. I think that's worked reasonably well.

You already have some helper methods to reduce repetition, but I think you can go even further by doing two things:

Make a function that deploys the message buffer program in a default setup. That function would combine this sequence of instructions here to get you a reasonably-initialized message buffer program.

Make a struct to hold the common addresses and such, and then make the helper functions methods on that struct.

guibescos · 2023-05-04T23:57:52Z

message_buffer/programs/message_buffer/src/instructions/resize_buffer.rs


    ctx.accounts
        .whitelist
        .is_allowed_program_auth(&allowed_program_auth)?;


Unrelated to this PR: but now that the message buffer account is only 1, should we move it to the Anchor context?

there were some issues with using the anchor context since our message buffer is dynaimcally sized but also needs to support zero_copy

@guibescos i was wrong about this. I was confusing the issues from working with an older implementation. I made a new PR to address this

guibescos · 2023-05-04T23:58:21Z

message_buffer/programs/message_buffer/src/instructions/resize_buffer.rs

            .realloc(target_size, false)
            .map_err(|_| MessageBufferError::ReallocFailed)?;
    } else {
+        // Check that account doesn't get resized to smaller than the amount of


Unrelated : why would we want to ever make a MessageBuffer account smaller?

i don't have a specific use-case other than if we upgraded to something in the future where we didn't need all the space that was currently allocated, then we wouldn't have to pull empty bytes for each MessageBuffer

.pre-commit-config.yaml

guibescos · 2023-05-05T00:02:23Z

message_buffer/programs/message_buffer/src/state/whitelist.rs

    pub bump:             u8,
    pub admin:            Pubkey,
+    // This is used by the `#[derive(InitSpace)]`
+    // to determine initial account size


Thanks for the comment. I didn't know this feature.

message_buffer/programs/mock-cpi-caller/tests/cases/test_create_msg_buffer.rs

ali-behjati · 2023-05-08T13:52:07Z

message_buffer/programs/mock-cpi-caller/tests/program_test/mod.rs

+pub fn verify_full_price_msg(
+    id: u64,
+    price: u64,
+    price_expo: u64,
+    ema: u64,
+    ema_expo: u64,
+    msg: &[u8],
+) -> Result<()> {
+    let mut cursor = Cursor::new(&msg[7..]);
+    let msg_id = cursor.read_u64::<BigEndian>().unwrap();
+    assert_eq!(id, msg_id);
+    let msg_price = cursor.read_u64::<BigEndian>().unwrap();
+    assert_eq!(price, msg_price);
+    let msg_price_expo = cursor.read_u64::<BigEndian>().unwrap();
+    assert_eq!(price_expo, msg_price_expo);
+    let msg_ema = cursor.read_u64::<BigEndian>().unwrap();
+    assert_eq!(ema, msg_ema);
+    let msg_ema_expo = cursor.read_u64::<BigEndian>().unwrap();
+    assert_eq!(ema_expo, msg_ema_expo);
+    Ok(())
+}
+
+pub fn verify_compact_price_msg(id: u64, price: u64, price_expo: u64, msg: &[u8]) -> Result<()> {
+    let mut cursor = Cursor::new(&msg[7..]);
+    let msg_id = cursor.read_u64::<BigEndian>().unwrap();
+    assert_eq!(id, msg_id);
+    let msg_price = cursor.read_u64::<BigEndian>().unwrap();
+    assert_eq!(price, msg_price);
+    let msg_price_expo = cursor.read_u64::<BigEndian>().unwrap();
+    assert_eq!(price_expo, msg_price_expo);
+    Ok(())
+}


I think it's good if we can remove them (otherwise we need to always make sure that it is correct itself). I don't have the full context on the code. Is there any reason that we are manually ser/de ing on the mock contract? I think we can use borsh (and it will make the code simpler)

@Reisen was working on a serializer/deserializer for the pythnet sdk. This was just to have some code to work off of that didn't rely on using the anchor client libraries

add rust-toolchain.toml to pin rust version, add integration tests, update cpi caller auth seeds

…stContext for less duplication

pythnet/message_buffer/programs/mock-cpi-caller/tests/program_test/mod.rs

guibescos · 2023-05-08T20:06:42Z

pythnet/message_buffer/programs/mock-cpi-caller/tests/cases/test_delete_buffer.rs

+#[tokio::test]
+#[should_panic]
+async fn fail_delete_buffer_invalid_admin() {
+    panic!()


Are this placeholders for future tests you'll write?

swimricky force-pushed the message-buffer/integration-tests branch from c623910 to cc2e14b Compare May 2, 2023 19:32

swimricky requested a review from jayantk May 3, 2023 13:22

swimricky marked this pull request as ready for review May 3, 2023 13:22

swimricky requested review from Reisen, ali-behjati and guibescos May 3, 2023 13:27

jayantk reviewed May 3, 2023

View reviewed changes

guibescos reviewed May 4, 2023

View reviewed changes

guibescos reviewed May 5, 2023

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

guibescos reviewed May 5, 2023

View reviewed changes

message_buffer/programs/mock-cpi-caller/tests/cases/test_create_msg_buffer.rs Outdated Show resolved Hide resolved

ali-behjati reviewed May 8, 2023

View reviewed changes

swimricky added 11 commits May 8, 2023 13:28

test(message-buffer): add rust integration tests

d3cf289

add rust-toolchain.toml to pin rust version, add integration tests, update cpi caller auth seeds

refactor(message-buffer): remove unused test ix

709b2c3

chore(message-buffer): clean up

71b9065

refactor(message-buffer): simple refactor

3231c47

test(message-buffer): refactor integration test structure

bfa5106

refactor(message-buffer): rename

c4fbe3f

fix(message-buffer): fix min size check when shrinking msg buffer

3152f0f

chore(message-buffer): cleanup

245c0b2

fix(message-buffer): resize borrow bug fix

e47d67b

test(message-buffer): refactor test util methods into MessageBufferTe…

454dc49

…stContext for less duplication

test(message-buffer): resolve merge conflicts from repo restructure

bacdc1b

swimricky force-pushed the message-buffer/integration-tests branch from 7adc4db to bacdc1b Compare May 8, 2023 18:06

chore(message-buffer): delete commented out code

ff290ce

guibescos reviewed May 8, 2023

View reviewed changes

pythnet/message_buffer/programs/mock-cpi-caller/tests/program_test/mod.rs Show resolved Hide resolved

guibescos reviewed May 8, 2023

View reviewed changes

guibescos approved these changes May 8, 2023

View reviewed changes

ali-behjati approved these changes May 9, 2023

View reviewed changes

swimricky merged commit 2e32a22 into main May 9, 2023

swimricky deleted the message-buffer/integration-tests branch May 9, 2023 17:57

[message-buffer 13/x] Rust Integration Tests #794

[message-buffer 13/x] Rust Integration Tests #794

Uh oh!

Conversation

swimricky commented May 2, 2023

Summary

Uh oh!

vercel bot commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swimricky commented May 3, 2023

Uh oh!

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

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!

guibescos May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ali-behjati May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vercel bot commented May 2, 2023 •

edited

Loading

guibescos May 4, 2023 •

edited

Loading

ali-behjati May 8, 2023 •

edited

Loading