Skip to content

An example contract for Solana #72

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 70 commits into from
Oct 12, 2022
Merged

Conversation

yhzhang0128
Copy link
Contributor

I have been working with Ali on this example contract. Comments are welcome and I will address them before merge.

@ali-behjati ali-behjati self-requested a review September 14, 2022 07:23
@ali-behjati ali-behjati requested a review from Reisen September 19, 2022 09:01
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.

One big thing I think we should definitely do is check the pyth account keys in the instruction. People are going to essentially copy-paste this code, so we should make sure that it's complete and secure.

Aside from that, I Ieft some minor comments around documentation etc.

@jayantk
Copy link
Contributor

jayantk commented Sep 19, 2022

Oh also you should install the pre-commit hooks here https://github.com/pyth-network/pyth-sdk-rs#pre-commit-hooks to fix this CI issue ^

@yhzhang0128
Copy link
Contributor Author

Following Jayant's comments, I made the following changes:

  1. Only the admin can initialize the loan information
  2. Loan2Value will check the Pyth accounts assuming the admin gave correct information
  3. Add more text in README about where/how Pyth SDK is used
  4. Use confidence interval in the example
  5. Test permission control by adding an unauthorized invocation of Init in the client

jayantk
jayantk previously approved these changes Oct 4, 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.

awesome! I left a bunch of minor comments. I think you're getting pretty close though, so I suggest merging this PR and then addressing the comments in separate small PRs.

@yhzhang0128
Copy link
Contributor Author

yhzhang0128 commented Oct 10, 2022

Hi Ali and Jayant, I resolved all your new comments. Specifically, I

  1. make quantities as client-side parameters instead of hard-coded value;
  2. use borsh serialization/deserialization for state.rs;
  3. update the value calculation using integers instead of floating points;
  4. resolve some renaming and small issues.

I also followed Ali's suggestion and rebase my branch with the latest main. I guess two things worth another eye:
(1) the value calculation in processor.rs;
(2) This PR now says 21 files changed and I'm a bit confused, because it seems to be comparing with the old main branch before I rebase with the latest main.

@ali-behjati ali-behjati merged commit 7df8baa into pyth-network:main Oct 12, 2022
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.

4 participants