-
Notifications
You must be signed in to change notification settings - Fork 127
Refactor README to include CCIP examples #29
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
base: main
Are you sure you want to change the base?
Conversation
👋 @andrejrakic, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the README to include CCIP examples, reorganizes Quick Start and prerequisites, and refreshes the Data Feed example. It also adds a new client
script and the minimist
dependency in package.json
.
- Added a
client
script andminimist
dependency in package.json - Expanded README with CCIP examples (token transfers, messaging, receiver)
- Streamlined Quick Start, Prerequisites, and testing instructions in README
Reviewed Changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
package.json | Introduced "client" script and added minimist dependency |
README.md | Added CCIP use-case examples, reorganized Quick Start and prerequisites |
Comments suppressed due to low confidence (2)
package.json:5
- [nitpick] The new
client
script name is quite generic and may not clearly convey its purpose. Consider renaming it to something more descriptive likerun-client
orstart-client
.
"client": "node client.js",
package.json:5
- A new
client
script was added, but there are no accompanying tests forclient.js
. Consider adding unit or integration tests to ensure the CLI remains robust.
"client": "node client.js",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can you update as per comments and then comment out your bashrc/zshrc env vars and make sure it all works correctly end to end using only .env?
1. **Set up a complete Solana development environment** with all necessary tools and dependencies | ||
2. **Deploy and interact with Chainlink Data Feeds** to fetch real-world price data directly in your Solana programs | ||
3. **Build cross-chain applications using CCIP** (Cross-Chain Interoperability Protocol) to send tokens and messages between Solana and EVM chains like Ethereum | ||
4. **Deploy a CCIP receiver program** that can accept cross-chain messages and tokens from other blockchains |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CCIP receiver to be deployed on SVM right?
May be worth putting in brackets that to see how to do EVM --> SVM they should go to the github.com/smartcontractkit/ccip-starter-kit-foundry/ or hardhat sk repos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw in target/deploy we have a "chainlink_solana_demo" --> maybe its time to update to a better/more descriptive name? Data Feeds demo/client/ something?
``` | ||
solana airdrop $(solana address --keypair id.json) --url https://api.devnet.solana.com | ||
|
||
Set up Anchor environment variables: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be moved up to the ENV Var setting section above?
Deploy to Devnet: | ||
|
||
```bash | ||
anchor deploy --provider.cluster devnet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think this defaults to using the id.json wallet right? all the previous steps do us it -- shouldnt this also? or does it read automatically from ANCHOR_WALLET? If so then when setting that env var we could educate the user by saying setting this evn var means they dont need to keep referring to id.json
``` | ||
anchor keys sync | ||
|
||
Deploy to Devnet: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should explain what the expected result is -- what are we deploying? Looks like we are deploying both contracts. I think i mentioned this issue in a previous comment.
When deploying the demo program i get
Deploying program "chainlink_solana_demo"...
Program path: /......./solana-starter-kit/target/deploy/chainlink_solana_demo.so...
Error: 35 write transactions failed
There was a problem deploying: Output { status: ExitStatus(unix_wait_status(256)), stdout: "", stderr: "" }.
|
||
## Advanced Configuration | ||
|
||
#### Token Management |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have already covered this in previous steps? This doesnt make intuitive sense presented like this. Also is it "may need to delegate" or they "must" delegate?
|
||
#### EVM Operations | ||
|
||
The starter kit also includes examples for initiating CCIP operations from EVM chains: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we were going to DRY and keep those in the other starter kits? if we have EVM --> SVM here then we double the maintenance (which is already required for HH and Foundry SK)
``` | ||
|
||
The integration test checks that the value of the specified price feed account (defaulted to SOL/USD) on Devnet is greater than 0. | ||
The integration test checks that the price feed data is accessible and returns valid values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesnt check for CCIP right? in which case "to verify everything is working correctly" is misleading. It should be clarified to say that the chainlink data feeds demo is working.
|
||
✅ **Set up a complete Solana development environment** with Chainlink integration | ||
✅ **Built programs that access real-world data** using Chainlink Data Feeds | ||
✅ **Created cross-chain applications** that can transfer tokens and messages between Solana and EVM chains |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specifically a sending program right? It would be clearer to clarify we have deployed a sender on Solana and a receiver on Solana.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add licence to package.json
This PR refactors README to include CCIP examples and refresh Data Feed example