-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add "Your Firsts" Example #14
Conversation
const signer = await testNodeWallet() | ||
const issueTokenAmount = 10000n | ||
// Deoloy `ShinyToken` contract and issue `10000` shiny tokens to `issueTokenTo` address. | ||
const shinyToken = await ShinyToken.deploy(signer, { |
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.
It might be confusing to educate new developers on deploying contracts using two different methods:
- Raw approach:
ShinyToken.deploy(...)
- Using deploy scripts:
deployer.deployContract(...)
But I don't have a better solution here.
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.
If we had to pick one maybe deployer.deployContract
is better? Since it is probably what's gonna be used in production.
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.
But i tried to avoid deployment scripts in the example, since it is another concept. Same for the AwesomeNFT
template id, it feels a lot easier that everything happens in one ts file.
WDYT?
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.
Let's keep it this way for now, I don't have any better ideas.
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.
LGTM 👍 , only one miner comment
your-firsts/README.md
Outdated
|
||
## Compile | ||
|
||
Compile the TypeScript files into JavaScript: |
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 it be Compile the contract files
?
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.
Nice catch, thanks!
To be used in the doc.