[accumulator-updater 3/x] Update Inputs Ix #741
Conversation
…rogram Initial layout for accumulator updater program. Includes mock-cpi-caller which is meant to represent pyth oracle(or any future allowed program) that will call the accumulator updater program. All implementation details are open for discussion/subject to change
…ctoring to address PR comments
…schemas & use zero-copy
# Conflicts: # accumulator_updater/Cargo.lock # accumulator_updater/programs/accumulator_updater/src/lib.rs # accumulator_updater/programs/mock-cpi-caller/Cargo.toml # accumulator_updater/programs/mock-cpi-caller/src/lib.rs # accumulator_updater/tests/accumulator_updater.ts
…r::initalize ix for consistency
rename schema to message & associated price messages, remove unncessary comments, changed addAllowedProgram to setAllowedPrograms
consolidate SetAllowedPrograms and UpdateWhitelistAuthority into one context
…cumulatorInput header
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
There was a problem hiding this comment.
new update_input logic
There was a problem hiding this comment.
refactor of AccumulatorInput & AccumulatorHeader account state
There was a problem hiding this comment.
refactor of whitelist related logic
There was a problem hiding this comment.
refactor of mock add_price
There was a problem hiding this comment.
new mock update_price logic
| Ok(()) | ||
| } | ||
|
|
||
| pub(crate) fn update(&mut self, params: UpdatePriceParams) -> Result<()> { |
There was a problem hiding this comment.
refactor besides PriceAccount.update()
accumulator_updater/programs/accumulator_updater/src/instructions/create_inputs.rs
Outdated
Show resolved
Hide resolved
| base_account: Pubkey, | ||
| data: Vec<Vec<u8>>, | ||
| account_type: u32, | ||
| account_schemas: Vec<u8>, |
There was a problem hiding this comment.
I think it would be nice if the interface of this function resembled a commonly-used data structure so that callers had a natural reference point for what to expect. I think the accumulator is most similar to a Map where the keys are (base_account, account_schema) and the values are arbitrary data. Given that, I would try to make the arguments map on to the function signature for inserting multiple items in a Map. E.g., I think a function signature like
/// Insert elements into the Map identified by the calling program and map_id.
/// Each element is written into a separate account (explain how to derive the PDA).
/// The caller of this instruction must pass those PDAs while calling this function in the same order as elements.
put_all(ctx, map_id: Pubkey, elements: <Vec<(u8, Vec<u8>)>>)
would make more sense. (map_id is base_account)
| ctx: Context<'_, '_, '_, 'info, EmitInputs<'info>>, | ||
| base_account: Pubkey, | ||
| data: Vec<Vec<u8>>, | ||
| account_type: u32, |
There was a problem hiding this comment.
do we need account_type at all? I don't remember exactly but i'm looking back at our previous convo and it's not mentioned there:
There was a problem hiding this comment.
this was for flexibility if we ever wanted to include different types of accounts into the accumulator aside from the price accounts - maybe even if it was something outside of pyth (i don't have any specific use-case in mind and not sure if we would ever want product or mapping accounts but I can remove this if we really don't need this)
There was a problem hiding this comment.
will remove for now
| let accumulator_input = AccumulatorInput::new( | ||
| AccumulatorHeader::new( | ||
| bump, | ||
| 1, //from CPI caller? |
There was a problem hiding this comment.
I think this program controls the header format (and therefore gets to set the version)
I would suggest extracting this value out to a constant like AccumulatorHeader::CURRENT_VERSION though so it's easy to bump all the places where it may be used.
|
|
||
| //TODO: update header? | ||
| accumulator_input.data = account_data; | ||
| accumulator_input.persist(ai)?; |
There was a problem hiding this comment.
I think the structure of this code should be:
phase 1: check that you have the right account key
phase 2: generate the accumulator_input and serialize it to a byte array
phase 3: given the byte array, write it to a new account or overwrite the existing account data.
that implementation is less error prone than this one -- in this version, we can easily forget to change one of the code paths for serialization and that will cause problems.
There was a problem hiding this comment.
fwiw i'm pretty sure we have code somewhere for doing the create/update w/ resizing if needed in some solana program somewhere. @drozdziak1 or @guibescos will be able to point you to it.
There was a problem hiding this comment.
I removed the resizing logic from the oracle but it's here pyth-network/pyth-client@90ba997#diff-6b3dac646ee82aa19e2da0df982aa54c40e2005422fa4fd5805d1b5e840d3ca5L86 . It's pretty straightforward with realloc
There was a problem hiding this comment.
will handle the re-sizing in a separate PR
| pub payer: Signer<'info>, | ||
| pub whitelist_verifier: WhitelistVerifier<'info>, | ||
| pub system_program: Program<'info, System>, | ||
| //TODO: decide on using optional accounts vs ctx.remaining_accounts |
There was a problem hiding this comment.
seems like the implementation using ctx.remaining_accounts above is pretty reasonable. i would go with that.
| @@ -18,6 +15,8 @@ declare_id!("Fg6PaFpoGXkYsidMpWTK6W2BeZ7FEfcYkg476zPFsLnS"); | |||
| pub mod accumulator_updater { | |||
| use super::*; | |||
|
|
|||
|
|
|||
| /// Initializes the whitelist and sets it's authority to the provided pubkey | |||
There was a problem hiding this comment.
maybe add Once initialized, the authority must sign all further changes to the whitelist.
| pub bump: u8, | ||
| pub version: u8, | ||
| // u32 for parity with pyth oracle contract | ||
| pub account_type: u32, |
There was a problem hiding this comment.
yeah we definitely don't need this -- we only send data from price accounts to other blockchains anyway.
| Self { header, data } | ||
| } | ||
|
|
||
| pub fn validate_account_info( |
There was a problem hiding this comment.
this doesn't seem like the right encapsulation of functionality -- it's a very specific function that can only be used in one place. I think a better function would be something like derive_pda_for(cpi_caller, base_account, account_schema) ?
| accumulator_input, | ||
| accumulator_size, | ||
| &ctx.accounts.payer, | ||
| &[accumulator_acc_seeds_with_bump!( |
There was a problem hiding this comment.
i'm a little confused about the use of this function w/ the bump vs. the one w/o the bump. It seems like you want there to be a single PDA for each key, right? if so, isn't the bump deterministically generated, so there shouldn't be a reason to save it? (is this an efficiency improvement? if so, does it not introduce security holes because there are now multiple PDAs that can pass the checks for the same key?)
There was a problem hiding this comment.
there's the "canonical bump" which is the first bump counting down from 255 that provides a valid PDA address (off the ed25519 curve). This is the bump that's returned with find_program_address. But there could technically be multiple bumps that create valid PDA addresses.
There's 2 ways to verify a PDA address:
- assume you're always using the canonical bump and check that
Pubkey::find_program_address(&[seeds]).0 == account.key() - store the bump in the account and check
account.key() == Pubkey::create_program_address(&[seeds, &[account.bump]])
2 is more efficient (marginally) since 1 could theoretically have to do a guess-and-check multiple times to find the first valid seed.
Regarding your concern about security holes - since only a program can create PDAs it owns, it shouldn't be possible to have multiple PDAs using the same seeds but different bumps. The accumulator-updater program will be the only program to create these PDAs and it's using the canonical bumps when the PDAs are first initialized.
Having 2 macros is a bit confusing so i'll remove the non-bump one since that's only ever used once
| // 44444 compute units | ||
| // AddPrice::invoke_cpi_anchor(ctx, account_data, PythAccountType::Price, account_schemas) | ||
| // 44045 compute units | ||
| AddPrice::emit_accumulator_inputs(ctx, account_data, PythAccountType::Price, account_schemas) |
There was a problem hiding this comment.
this isn't necessary here, right? the update price instruction will also create the accounts now if they're missing, so we only have to call emit_inputs there?
There was a problem hiding this comment.
oh, I think this question may come down to a misunderstanding of how the oracle program works. In the oracle program, when you add a price account, it doesn't actually contain any price information. it simply allocates the space for putting prices in later. So there wouldn't even be any data to emit. (whereas this version of add price actually puts some data in the price account).
anyway, since this is just a test program, so I would scope it down to the minimum necessary code to demonstrate how to invoke the accumulators & test all the needed functionality. pretty sure you don't need both add price and update price to do that.
There was a problem hiding this comment.
oh got it thanks for clarifying that. i'll update this and the relevant tests/code and scope it down
There was a problem hiding this comment.
I left the add_price & cpi call in for now but left a comment describing the actual behavior in the oracle program.
accumulator_updater/programs/mock-cpi-caller/src/instructions/add_price.rs
Show resolved
Hide resolved
Remove account_type, rename emit_inputs to put_all to be more similar to a Map interface, added AccumulatorHeader::CURRENT_VERSION
Summary
Implement
mock-cpi-program::update_price&accumuulator-updater::update_inputsixsUpdates
TODOs
create_inputs&update_inputsix into one (see Notes below)invoke_cpi_solanato useinstruction::new_with_borshforinstruction_dataserializationTesting
cd accumulator_updater && anchor test
Notes
delete_inputsdo we need to handle deleting inputs outside of a CPI call? one use-case would be if an accumulator message schema was being deprecated/no longer used