Skip to content
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 smart contract and deploy script #2

Merged
merged 2 commits into from
Dec 20, 2022

Conversation

fuzzy31u
Copy link
Contributor

Issue

#1

Overview

  • RFC1155でのNFT実装
  • 確認事項コードコメントします

Copy link
Contributor Author

@fuzzy31u fuzzy31u left a comment

Choose a reason for hiding this comment

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

@rlho WIPで方向性確認PR投げましたのでご確認お願いします!

Comment on lines +1 to +2
artifacts
cache
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hardhat compile 時に生成されるこれらってコミット対象外であってます・・?

Copy link
Contributor

Choose a reason for hiding this comment

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

はい!あってます!!

uint256 public constant ITEM_1 = 1;

constructor() ERC1155("https://rlho.github.io/nft_sample/{id}}.json") {
_mint(msg.sender, ITEM_1, 1, "");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

試しということでシンプルに _mint のみ書きましたがどういった条件でどのNFTに対するmintがいくつ発行されるなどの仕様詳細を、お手すきで教えてくださーい!

Copy link
Contributor

Choose a reason for hiding this comment

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

ありがとうございます!
個数とtoken_idは引数で渡せるようにして欲しいです〜!


module.exports = {
solidity: "0.8.17",
defaultNetwork: "mumbai",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mumbaiで良いのでしょうか・・

Copy link
Contributor

Choose a reason for hiding this comment

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

今回はEtheriumを考えてます!

Comment on lines +16 to +17
url: TESTNET_API_URL,
accounts: [TESTNET_PRIVATE_KEY]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

testnetに何を使ったら良いかわかっておらず、Goerli?などでAPI URLを取得する必要があるようですがこちらありますでしょうか!

Copy link
Contributor

Choose a reason for hiding this comment

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

DMで送ります!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mainnetとtestnetのAPI_URLとPRIVATE_KEYの2つでいいのかな?を .env に入れてもらう形ですー

@@ -0,0 +1,2 @@
const { expect } = require("chai");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://hardhat.org/hardhat-runner/docs/guides/test-contracts
こちらを参考にこれから書いていこうと思います。
NFTの仕様をざーっくりで良いので教えていただければと思ってますb

@fuzzy31u fuzzy31u marked this pull request as draft November 30, 2022 08:46
Copy link
Contributor

@rlho rlho left a comment

Choose a reason for hiding this comment

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

@fuzzy31u
全体的に方向性合ってます!ありがとうございます!
多分仕様の詳細お伝えできてないと思うのでいちどお話ししましょう〜!

uint256 public constant ITEM_1 = 1;

constructor() ERC1155("https://rlho.github.io/nft_sample/{id}}.json") {
_mint(msg.sender, ITEM_1, 1, "");
Copy link
Contributor

Choose a reason for hiding this comment

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

ありがとうございます!
個数とtoken_idは引数で渡せるようにして欲しいです〜!


module.exports = {
solidity: "0.8.17",
defaultNetwork: "mumbai",
Copy link
Contributor

Choose a reason for hiding this comment

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

今回はEtheriumを考えてます!

Comment on lines +16 to +17
url: TESTNET_API_URL,
accounts: [TESTNET_PRIVATE_KEY]
Copy link
Contributor

Choose a reason for hiding this comment

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

DMで送ります!

@fuzzy31u fuzzy31u changed the title [WIP] Add smart contract and deploy script Add smart contract and deploy script Dec 17, 2022
@fuzzy31u fuzzy31u force-pushed the feat/add-nft-contract branch from e7ee017 to 7c423ab Compare December 17, 2022 13:07
Copy link
Contributor Author

@fuzzy31u fuzzy31u left a comment

Choose a reason for hiding this comment

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

@rlho mint functionのところ、こんな実装で良きでしょうか?

}

function mintDocuments(num) private{
require(balanceOf(msg.sender, ITEM_DOCUMENTS) == 0, "You already have a Documents.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

こういったバリデーション処理は要りますか?

Copy link
Contributor

Choose a reason for hiding this comment

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

全然仕様をお伝えできてなくてすみません><
バリデーション助かります!
今回は複数持てるようにしようかと思ってるのでちょっと違う形になりそうです!


constructor() ERC1155(NFT_URI) {}

function mint(token_id, num) public{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clientからはこちらを呼んでもらうイメージです!

Comment on lines +16 to +17
url: TESTNET_API_URL,
accounts: [TESTNET_PRIVATE_KEY]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mainnetとtestnetのAPI_URLとPRIVATE_KEYの2つでいいのかな?を .env に入れてもらう形ですー


module.exports = {
solidity: "0.8.17",
// defaultNetwork: "hardhat",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

サンプルコード見てるとここはtestかlocalっぽいnetworkを入れてるケースが多そうに見えましたが何を指定すべきかわからずコメントアウトしておりますー。testnetにします??

Copy link
Contributor

Choose a reason for hiding this comment

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

そうですね!デフォルトはtestnetにしておきたいです!

Comment on lines +15 to +19
if (token_id == ITEM_DOCUMENTS) {
mintDocuments(num);
} else if (token_id == ITEM_FLOWER_1) {
mintFlower1(num);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solidity自体にはswitch文がないんですね・・?ググったらassemblyとやらでswitch使えると書いてありましたが一応ifにしておきました。もっと良い書き方あるのかな??

Copy link
Contributor

Choose a reason for hiding this comment

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

switchないんですよね…
今回はシンプルなのでif文で良いと思います!

Copy link
Contributor

@rlho rlho left a comment

Choose a reason for hiding this comment

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

LGTMです 👍 ✨
ここまで書いてくださりめちゃくちゃ助かりました…!

Comment on lines +15 to +19
if (token_id == ITEM_DOCUMENTS) {
mintDocuments(num);
} else if (token_id == ITEM_FLOWER_1) {
mintFlower1(num);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

switchないんですよね…
今回はシンプルなのでif文で良いと思います!

}

function mintDocuments(num) private{
require(balanceOf(msg.sender, ITEM_DOCUMENTS) == 0, "You already have a Documents.")
Copy link
Contributor

Choose a reason for hiding this comment

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

全然仕様をお伝えできてなくてすみません><
バリデーション助かります!
今回は複数持てるようにしようかと思ってるのでちょっと違う形になりそうです!


module.exports = {
solidity: "0.8.17",
// defaultNetwork: "hardhat",
Copy link
Contributor

Choose a reason for hiding this comment

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

そうですね!デフォルトはtestnetにしておきたいです!

@rlho rlho marked this pull request as ready for review December 20, 2022 23:17
@rlho rlho merged commit 5e27708 into cr3atorsstudio:main Dec 20, 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.

2 participants