Skip to content

Add resize_price_account support; create .dockerignore #41

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 3 commits into from
Aug 7, 2023

Conversation

drozdziak1
Copy link
Contributor

@drozdziak1 drozdziak1 commented Jul 31, 2023

Motivation

This is the main changeset responsible for price account resizing in program-admin. Account resizing is realized as a new CLI subcommand (resize-price-accounts-v2). The new command performs account discovery similar to sync, finds accounts with insufficient data size and expands them using the relevant on-chain instruction. The reason for a separate subcommand (and not expanding sync) is that resizing will likely be performed only once per oracle deployment, as with current program-admin code, new price accounts are already allocated to use expanded PriceAccountV2 size.

In order to perform tests in integration, a way to allocate v1-sized price accounts was necessary. To support that, the sync subcommand got an option to allocate new price accounts using v1 size (--allocate-price-v2, uses v1 size when set to false). This way, when resize_price_account is called on chain, a true resize is performed. The resize would be skipped otherwise, as the on-chain instruction exits early if there's enough space already.

Additionally, a .dockerignore file is created from .gitignore to prevent Python runtime artifacts and locally-stored secrets from being copied into Docker storage.

Summary of changes

  • cli.py - Add new subcommand, add choice between v1 and v2 account size for sync price account allocation
  • __init__.py - Add logic for finding, filtering and expanding v1 price accounts;
  • instructions.py - Implement resize_price_account on-chain instruction
  • util.py - Differentiate between V1 and V2 account sizes in constants
  • parsing.py - Make parse_price aware of the extra price components for v2 prices, ensure that trailing PriceCumulative data is not misinterpreted as a price component
  • pyproject.toml - Bump package version to v0.1.2
  • .dockerignore - copied from .gitignore

Testing

  • The new subcommand works well within the integration environment, correctly identifying v1 accounts and v2 ones. The resize transactions succeed and do not break other services in the Tilt environment (agent continues to publish).
  • Existing unit tests continue to pass

Review highlights

  • parsing.py - I decided against parsing the trailing data after expanded price component array, because the data is not relevant to administrative tasks on price accounts. It is indicated in the code comments.

@drozdziak1 drozdziak1 requested review from thmzlt, guibescos and jayantk and removed request for thmzlt and guibescos July 31, 2023 16:03
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.

seems fine to me, though I can't say whether or not the txs are constructed exactly right. This could definitely use some tests, but maybe those go in a different repo? I've sort of lost the thread on the testing story here.

Anyway, I'm going to approve this, but please do add tests as a follow up.

Copy link
Contributor

@thmzlt thmzlt left a comment

Choose a reason for hiding this comment

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

I guess the new code is covered because the flag value defaults to true.

@drozdziak1
Copy link
Contributor Author

drozdziak1 commented Aug 7, 2023

Changes since last review

  • parsing.py - Opted for using the existing num_components field for price component parsing, which I previously missed. This greatly simplifies the change to parsing.py, making extensive testing less necessary.
  • test_sync.py - changed all instances of sync_from_files to use allocate_price_v2=False - This gives opportunity to parsing.py code to encounter shorter v1 buffers, ensuring that parsing never goes out of bounds
  • .dockerignore, .gitignore - Exclude Python package builds from Docker and Git, located in dist/

@drozdziak1 drozdziak1 merged commit 622998e into main Aug 7, 2023
@drozdziak1 drozdziak1 deleted the drozdziak1/resize-price-account-support branch August 7, 2023 15:15
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.

3 participants