Skip to content

Add batch price update instruction to Oracle #142

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 4 commits into from
Feb 14, 2022

Conversation

tompntn
Copy link
Contributor

@tompntn tompntn commented Feb 7, 2022

Overview

This PR adds a new instruction to the Oracle: cmd_upd_price_no_fail_on_error. This is exactly the same as cmd_upd_price, but never fails the instruction, and therefore the transaction, even if the update is unsuccessful. This is because we will soon send update instructions in batches, and want to prevent a failure in one price feed blocking updates of other prices.

Approach

  • Add the cmd_upd_price_no_fail_on_error instruction.
  • Change pythd to send cmd_upd_price_no_fail_on_error JPRC requests instead of cmd_upd_price.

Testing

  • pythd changes: the existing pythd integration tests for the JRPC update_price request will now cover the new e_cmd_upd_price_no_fail_on_error code paths.
  • Oracle changes: new Oracle tests for the cmd_upd_price_no_fail_on_error instructions have been added, demonstrating the optimistic failure semantics.

Caveats

All price update transactions will appear on-chain, even if they are unsuccessful. This means publishers will have to rely on inspecting publishing slots to be sure if their updates were successful.

@tompntn tompntn force-pushed the oracle-batch-upd-price branch 4 times, most recently from c53b763 to 62d85c8 Compare February 10, 2022 14:09
@tompntn tompntn marked this pull request as ready for review February 10, 2022 14:25
@tompntn tompntn requested review from jayantk and Reisen February 10, 2022 14:27
jayantk
jayantk previously approved these changes Feb 10, 2022
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.

LGTM. I think there may be an issue with the compute limit (see inline), but addressing that would be a small change to the client code so not a big deal. Please let @Reisen review as well before merging.

}

// Verify that the correct number of accounts have been passed
if ( (prm->ka_num != 3 && prm->ka_num != 4) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(not related to this change) would you happen to know why this lets you submit 4 accounts ? seems like a weird feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question - I'm not sure. AFAICT pythd only ever sends 3 accounts, and the Oracle program doesn't ever use the 4th one. I can't see anywhere this is used, but maybe I'm missing something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think an old version of the onchain program took a 'parameter' account which was removed in cd9dc8c

jayantk
jayantk previously approved these changes Feb 11, 2022
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.

Nice this should definitely work. Again would still like @Reisen to review before merging.

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.

well, at this point, it's a tiny change, so feel free to merge even without @Reisen

@tompntn tompntn merged commit e6997cc into pyth-network:main Feb 14, 2022
@tompntn tompntn deleted the oracle-batch-upd-price branch February 14, 2022 09:27
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