-
Notifications
You must be signed in to change notification settings - Fork 56
CCT tasks #36
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?
CCT tasks #36
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 renames CCIP v1.5 tasks to CCT tasks across code imports and documentation.
- Imported the new
cct-tasks
file in the task index - Removed the deprecated CCIP 1.5 task import from Hardhat config and streamlined environment URL expressions
- Updated README section header, description, and parameter list to reference CCT
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
tasks/index.ts | Added import './cct-tasks' to load the renamed tasks |
hardhat.config.ts | Removed ccip-1_5-tasks import and converted multi-line URL checks to single-line expressions |
README.md | Changed "CCIP 1.5 Token Pool Tasks" to "CCT Standard", refreshed overview text, and adjusted parameter list formatting |
Comments suppressed due to low confidence (3)
README.md:779
- [nitpick] Consider renaming this section header from 'CCT Standard' to 'CCT Tasks' to align with the PR’s goal of renaming tasks and improve clarity.
## CCT Standard
README.md:899
- [nitpick] Ensure consistent markdown list formatting under the 'Parameters:' section by aligning all bullets with a single '-' prefix and uniform indentation.
- `--network`: The network where the _local_ pool is deployed.
hardhat.config.ts:51
- Use the nullish coalescing operator (
??
) to simplify defaulting environment variables:url: ETHEREUM_SEPOLIA_RPC_URL ?? ''
.
url: ETHEREUM_SEPOLIA_RPC_URL !== undefined ? ETHEREUM_SEPOLIA_RPC_URL : "",
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.
Thanks! LGTM subject to review of comments provided
--remote-pool <remotePoolAddress> | ||
--remote-token <remoteTokenAddress> | ||
npx hardhat configure-pool --network <localNetworkName> | ||
--local-pool <localPoolAddress> |
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.
just saw that for sending tokens we use source
and destination
language but here we're using local
and remote
should we not be consistent with public docs etc and stick to source/dest?
depending on whether you filled the [`SourceMinter.sol`](./contracts/cross-chain-nft-minter/SourceMinter.sol) contract with `Native` or `LINK` in step number 3. | ||
|
||
## CCIP 1.5 Token Pool Tasks | ||
## CCT Standard |
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.
check TOC at top-- i think its broken
|
||
```shell | ||
npx hardhat setup-burn-mint-pool --network <networkName> | ||
npx hardhat setup-burn-mint-pool --network <networkName> |
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.
for step 2 and 3
-
should we clarify that BOTH pools are not required?
Devs are likely to just follow them in sequence...and should understand that they may want to make a choice - because there is a choice? -
since the configure pool step will require deployment on a remote pool we should prepare them for that in this step. For example if I am deploying only on source when i get to the configure pool step i will discover the --remote-pool (destination) address is needed...but i've only done on source chain! So we should tell user here to deploy BOTH tokens and pools on both networks by the time they get to step 4
--destination-network <destinationNetworkName> | ||
--receiver <receiverAddress> | ||
--amount <amountToSend> | ||
npx hardhat send-ccip-tokens --network <sourceNetworkName> |
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 we make this easy to paste multiline by having a \
at the end of each line?
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.
Idea: I've been using fs.writeFilesync in workshops and other tooling to write a "info.json" file to project root. This file saves all the output from each steps in a JSON. eg (by network) the CCT token address, pool address etc
Because people often clear console and lose the data they need in subsequent steps. So if tell them to collect it from the notes.json file they can clear console, take a break etc without losing their "context" of the step.
Is it worth adding to both SKs?
This PR renames CCIP v1.5 tasks to CCT tasks