Skip to content

Accumulator Support and Integration Testing #62

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.

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

Merged
merged 28 commits into from
Apr 30, 2023

Conversation

drozdziak1
Copy link
Contributor

@drozdziak1 drozdziak1 commented Apr 19, 2023

This builds on Jayant's WIP PR for accumulator support. Changes:

  • Accumulator program binary added to integration tests and passed to agent in test keystore
  • log all transaction statuses to DEBUG, log failing ones to WARN

@drozdziak1
Copy link
Contributor Author

drozdziak1 commented Apr 20, 2023

more updates in tests as in price updates. The current blocker seems to be tripping the aggregate update, mainly due to the 20-publisher min_pub default. Here's some options:

  • mock 20 publishers to meet the default min_pub value for triggering aggregate update and then the CPI (the hard, most legit way)
  • hand-roll a set_min_publishers transaction to set the value to 1 on one of the mock symbols (easier, still not cursed)
  • use a cursed oracle binary that always updates the accumulator just to see how the cross call is doing (might be the easiest, costs build time and invisible conscience points)

@drozdziak1
Copy link
Contributor Author

drozdziak1 commented Apr 21, 2023

Summary of changes:

  • accumulator_updater.so - artifact after commenting out whitelisting checks and disabling whitelist account initialization checks
  • oracle.so - artifact after changing crosscall's AccountMeta::new_readonly to AccountMeta::new for accumulator_data (making it writable for the updater)
    *test_integration.py - moved the long sleep past assertions, fixed wrongly deploying oracle.so as accumulator. Note: The long sleep will still prevent the green test status, assertions will pass and reach the sleep() though

@drozdziak1
Copy link
Contributor Author

drozdziak1 commented Apr 25, 2023

Changes:

  • updated oracle.so from latest tip of accumulator-cpi
  • regenerated message_buffer with anchorpy 0.14.0. This version uses the same solana/solders version as the test. The previous existing generated code contained too new solana/solders usage which clashed with older solana/solders deps in integration test. Specified --pdas to automate the static whitelist PDA, verified that the generated seeds are correct.
  • finished calls to initialize() and set_allowed_programs(), logged auth PDA address. Both succeed. Note: the accumulator takes auth PDAs, not bare program IDs for set_allowed_programs, despite the name
  • added oracle_auth_pda to exporter.rs oracle call, logged PDA address

Status:
We get a signer escalation error despite passing the correct PDA (as logged in agent, pytest setup and ultimately in ledger/solana_logs/stdout). The seemingly escalated signer is the auth PDA.

@drozdziak1
Copy link
Contributor Author

drozdziak1 commented Apr 27, 2023

Finalized summary of changes

This changeset updates Solana program dependencies and implements client code to call into the oracle with accumulator CPI support. The majority of changes accomodates evolving integration testing setup.

Review highlights

  • integration-tests/message_buffer_client_codegen/ is AnchorPy codegen and can be ignored

Agent changes

  • Accept a new <network>.key_store.accumulator_key_path option, enabling the accumulator CPI in the called oracle
  • Exporter: additional create_instruction_with_accumulator implementation containing the correct accounts to perform accumulator CPI, improved failed transaction logging, compute unit limit bumped 20k -> 40k

Documentation

  • Unified README.md at repo root, described integration tests in detail
  • Added accumulator config option to config/config.toml, tweaked some of the comments

Integration tests

  • Add message_buffer.so containing the latest accumulator program from pyth-crosschain's main
  • Add message_buffer_idl.json, the IDL for the *.so
  • Add integration-tests/message_buffer_client_codegen/, an Anchor client generated with anchorpy
  • Update oracle.so to the tip of accumulator-cpi on pyth-client

test_integration.py

  • Deploy both oracle and accumulator using solana-test-validator --bpf-program <addr> <bin.so> instead of solana-program-deploy. This lets us keep the real private key hidden, while keeping the program ID consistent across environments. This also puts Anchor's program ID mismatch errors to rest.
  • Add USE_ACCUMULATOR env for switching accumulator features on in all parts of the test setup.
  • Add SOLANA_TEST_VALIDATOR env for supplying alternative validator binaries - this enables us to use a prebuilt pythnet validator easily and work with its accumulator outputs.
  • Update program-admin - this adds the min_publishers option on which the test relies for correct aggregate price values
  • Fixture a full initialization for the message_buffer program - this does accumulator initialization, complete with permissions and buffer allocation
  • Fixture agent config to supply accumulator key when USE_ACCUMULATOR is in effect
  • Add a disabled test with infinite loop for live experimenting on the running test suite

Miscellaneous

  • Add .dockerignore
  • Fail agent startup when config does not exist
  • Log working directory and config path in agent
  • Guard unexpected *.so and accumulator IDL changes with a md5sum hook

@drozdziak1 drozdziak1 changed the title [WIP] Drozdziak1/accumulator integration test Accumulator Support and Integration Testing Apr 27, 2023
Copy link
Contributor

@jayantk jayantk left a comment

Choose a reason for hiding this comment

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

Awesome. I like the toggleable behavior for testing against different environments. My main ask here is for some organization and documentation:

  1. write down the env flags in the readme and how/when you should use them.
  2. would also be nice to put the binaries into a folder like program_binaries/
  3. see inline comments about the autogenned python/idl stuff

that's all easy stuff so i'm going to approve this and trust you to fix those issues.

@@ -0,0 +1,573 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

can you put this in an idl/ folder or something? and similarly put the autogenned python into a folder like autogen and provide instructions for how to update that code from the IDL in the readme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gathered all artifacts in program-binaries, renamed the JSON to message_buffer_idl.json

yield

'''
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 bring back the logs now. this was commented out as a hacky thing for some debugging i think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

path = os.path.join(agent_keystore_path, "accumulator_program_key.json")

with open(path, 'w') as f:
f.write(MESSAGE_BUFFER_PROGRAM)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a string-only json file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, we do this for other addresses too, I think we noticed this on one occasion and decided to remain crappy but consistent

&price_info,
network_state.current_slot,
)?;
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

delete commented out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -144,6 +144,8 @@ mod key_store {
pub program_key_path: PathBuf,
/// Path to the public key of the root mapping account, relative to the root
pub mapping_key_path: PathBuf,
/// Path to the public key of the accumulator program, relative to the root.
pub accumulator_key_path: Option<PathBuf>,
Copy link
Contributor

Choose a reason for hiding this comment

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

(it's confusing that these pubkeys are in separate files as opposed to simply being strings in the config, but this is consistent with the other keys so it's fine)

@@ -128,7 +129,7 @@ impl Default for Config {
inflight_transactions_channel_capacity: 10000,
transaction_monitor: Default::default(),
// The largest transactions appear to be about ~12000 CUs. We leave ourselves some breathing room.
compute_unit_limit: 20000,
compute_unit_limit: 40000,
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 know that this is enough now? (i'll make a note in our roadmap doc to check)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is configurable, but I agree that we should make sure.

&accumulator_program_key,
);

debug!(self.logger, "Oracle Auth PDA"; "address" => oracle_auth_pda.to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

you probably don't want this log statement all the time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@drozdziak1 drozdziak1 merged commit 77a858c into main Apr 30, 2023
@drozdziak1 drozdziak1 deleted the drozdziak1/accumulator-integration-test branch April 30, 2023 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants