Change the branch name to canvasOnchain and also write a readme file in the hardhat folder for description #32

Abidoyesimze · 2024-12-05T04:42:57Z

Description

Concise description of proposed changes, We recommend using screenshots and videos for better description

Additional Information

I have read the contributing docs (if this is your first contribution)
This is not a duplicate of any existing pull request

Related Issues

Closes #{issue number}

Note: If your changes are small and straightforward, you may skip the creation of an issue beforehand and remove this section. However, for medium-to-large changes, it is recommended to have an open issue for discussion and approval prior to submitting a pull request.

Your ENS/address: 0x6BF7d6b94282BD48ff458599aDafA268BcB009FF

vercel · 2024-12-05T04:43:01Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
batch11.buidlguidl.com	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 17, 2024 10:22am

vercel · 2024-12-05T04:43:03Z

@Abidoyesimze is attempting to deploy a commit to the BuidlGuidl Team on Vercel.

A member of the Team first needs to authorize it.

melanke · 2024-12-05T13:50:25Z

packages/hardhat/contracts/CanvasOnchain.sol

The filename is not matching the contract name

melanke · 2024-12-05T13:50:59Z

packages/hardhat/deploy/canvasOnchain.ts

here it's called canvasOnChain

Check everything to be sure it's the same name everywhere

melanke · 2024-12-05T13:55:39Z

packages/hardhat/hardhat.config.ts

@@ -33,7 +34,7 @@ const config: HardhatUserConfig = {
      },
    ],
  },
-  defaultNetwork: "localhost",
+  defaultNetwork: "sepolia",


I usually don't change this, because unwarned people might deploy to the incorrect network by mistake. When I want to deploy to another network I add the flag on the command:

yarn deploy --network sepolia

phipsae · 2024-12-07T04:00:16Z

packages/hardhat/hardhat.config.ts

@@ -54,7 +55,7 @@ const config: HardhatUserConfig = {
      accounts: [deployerPrivateKey],
    },
    sepolia: {
-      url: `https://rpc2.sepolia.org`,
+      url: `https://eth-sepolia.g.alchemy.com/v2/FIQ1qwifmra7ZqdkVHnZ2lHQAKG8j4Yd`,


As a general rule, never include an API key in the code of a GitHub repository. Instead, use the .env file. Alternatively, you can use public RPCs, such as those available here: https://chainlist.org/chain/11155111.

melanke · 2024-12-05T13:57:32Z

packages/hardhat/package.json

@@ -7,6 +7,7 @@
    "check-types": "tsc --noEmit --incremental",
    "compile": "hardhat compile",
    "deploy": "hardhat deploy",
+    "deploy:canvas": "hardhat run deploy/canvasOnchain.ts --network sepolia",


do we really need another script? Isn't the deploy script deploying everything that is on the deploy folder?

Will make changes to that it's when i'm having issue with the network but forget to remove it later on

melanke · 2024-12-05T13:58:54Z

packages/nextjs/scaffold.config.ts

@@ -10,7 +10,7 @@ export type ScaffoldConfig = {

 const scaffoldConfig = {
  // The networks on which your DApp is live
-  targetNetworks: [chains.optimism],
+  targetNetworks: [chains.sepolia],


Did you deploy BatchRegistry to sepolia as well?

I am unable to even deploy to sepolia now the gas is high right now

I have made all the changes but still having issue deploying sepolia is too high and when trying to use optimism it's giving me issue i don't understand pls can you help me to deploy it from your side

melanke · 2024-12-05T16:20:56Z

@Abidoyesimze there's still some things called CanvasOnChain and others PixelCanvas

melanke · 2024-12-05T16:24:53Z

packages/hardhat/Readme.md

+getCanvasSnapshot(): Returns entire canvas state
+
+3. Initialization
+Test the CanvasOnchain contract


Suggested change

Test the CanvasOnchain contract

Test the PixelCanvas contract

melanke · 2024-12-05T16:25:07Z

packages/hardhat/test/CanvasOnchain.ts

This filename

phipsae

Thanks a lot for the whole effort and the smart contract!
Looking forward to see how it is then implemented in the front end :)

Below you'll find comments, please let me know if you have any questions.
(I haven't reviewed the test file).

phipsae · 2024-12-07T04:00:16Z

packages/hardhat/hardhat.config.ts

@@ -54,7 +55,7 @@ const config: HardhatUserConfig = {
      accounts: [deployerPrivateKey],
    },
    sepolia: {
-      url: `https://rpc2.sepolia.org`,
+      url: `https://eth-sepolia.g.alchemy.com/v2/FIQ1qwifmra7ZqdkVHnZ2lHQAKG8j4Yd`,


As a general rule, never include an API key in the code of a GitHub repository. Instead, use the .env file. Alternatively, you can use public RPCs, such as those available here: https://chainlist.org/chain/11155111.

phipsae · 2024-12-07T04:10:19Z

yarn.lock

Since you are adding @nomicfoundation/hardhat-network-helpers, that should be the only change made here.

package.json

phipsae · 2024-12-07T04:20:05Z

packages/hardhat/hardhat.config.ts

@@ -7,6 +7,7 @@ import "@typechain/hardhat";
 import "hardhat-gas-reporter";
 import "solidity-coverage";
 import "@nomicfoundation/hardhat-verify";
+import "@nomicfoundation/hardhat-network-helpers";


Why would we need this import here?

phipsae · 2024-12-07T04:41:41Z

packages/hardhat/deploy/PixelCanvas.ts

Why make things more complicated than necessary :)
There is already a deploy script named 00_deploy_your_contract.ts file which you can use.
Take the following code, copy it and adapt it for your contract:

await deploy("BatchRegistry", { from: deployer, // Contract constructor arguments args: [deployer, BATCH_NUMBER], log: true, // autoMine: speeds up deployment on local networks by auto-mining the deployment transaction. No effect on live networks. autoMine: true, });

Currently, the contracts are not being copied to the deployedContract.ts file in the Next.js project as they should. (Deploy scripts need to run before 99_generateTsAbis.ts.)

Let me know if you have any questions!

packages/hardhat/contracts/PixelCanvas.sol

phipsae · 2024-12-07T05:01:24Z

packages/hardhat/contracts/PixelCanvas.sol

+        for (uint256 x = 10; x < 20; x++) {
+            for (uint256 y = 10; y < 50; y++) {
+                canvas[x][y] = Pixel({
+                    author: owner(),


You could also use in the initialization msg.sender as the author.

phipsae · 2024-12-07T05:11:14Z

packages/hardhat/contracts/PixelCanvas.sol

+    }
+
+    // 2D Mapping to store pixels
+    Pixel[CANVAS_WIDTH][CANVAS_HEIGHT] public canvas;


Use a mapping here instead. That is more gas-efficient and flexible, storing the whole array from the beginning a is not the ideal solution. The key advantages of a mapping compared to a fixed-size array are:

Lower Deployment Cost: There's no need to initialize the entire array, reducing upfront costs.

Higher Gas Efficiency: It only stores modified pixels, making it ideal for canvases where most pixels remain unchanged

Consider what additional functions we might not need if we switch to using a mapping.

phipsae

Thanks for the changes!

You still need to add a deploy script (important for reviewers to deploy and test it themselves).
Also, just a small comment below about the withdraw and how to send ether.

In general, please ensure your PR has a clear and descriptive title and description (file out the template form). For example, "Change the branch name to canvasOnchain and write a README file in the Hardhat folder for description" would be more suitable as a commit message. That's what this course is also about :)

phipsae · 2024-12-12T07:46:02Z

packages/hardhat/contracts/PixelCanvas.sol

+        require(balance > 0, "No funds to withdraw");
+
+        // Transfer the balance to the owner
+        payable(owner()).transfer(balance);


Not anymore the recommenced way to send ether.

https://www.cyfrin.io/glossary/sending-ether-transfer-send-call-solidity-code-example

package.json

phipsae · 2024-12-12T08:17:29Z

yarn.lock

No changes to this file.

melanke · 2024-12-14T14:48:37Z

packages/hardhat/deploy/00_deploy_your_contract.ts

+  // const batchRegistry = await hre.ethers.getContract<Contract>("BatchRegistry", deployer);
+  // console.log("\nBatchRegistry deployed to:", await batchRegistry.getAddress());
+  // console.log("Remember to update the allow list!\n");


we still need to deploy the BatchRegistry, right?

I actually don't have idea about it that's why i commented it out till @phipsae replied

As a reviewer of the PR, I would first test it in my local environment, which involves deploying all the contracts (including batchRegistry). Therefore, you should add the deployment of your new contract to the existing script (so you leave the script as it is and add the additional deploy code).

phipsae

See below my comments.
As soon as you have the deploy script, I will look at the contract.

Thank you!

phipsae · 2024-12-15T07:04:09Z

package.json

No changes to this file, pls.

phipsae · 2024-12-15T07:05:37Z

packages/hardhat/deploy/00_deploy_your_contract.ts

See my comment in the conversation.
Leave the script as it is, just add the deploy contract block to it (like it is done for batch registry).

phipsae

Thanks for the contract! I deployed the contract to optimism and made a few changes so we can finally merge it.

For next time, it would be great if you could run yarn deploy before pushing the code. It didn't work because you were passing an argument into the constructor, but the contract doesn’t accept one. :))

Abidoyesimze added 3 commits December 4, 2024 12:30

create the CanvasOnchain contract and write the test

6a2a306

remove the reset function

8ba8d1d

add a readme file for the description in the hardhat folder

5a7ec9d

vercel bot deployed to Preview December 5, 2024 04:45 View deployment

write the deploy script and deploy to hardhat localhost

b9ad0e1

vercel bot deployed to Preview December 5, 2024 13:36 View deployment

melanke suggested changes Dec 5, 2024

View reviewed changes

updates

f61835f

vercel bot deployed to Preview December 5, 2024 14:24 View deployment

melanke reviewed Dec 5, 2024

View reviewed changes

resolve issue

29bc88a

vercel bot deployed to Preview December 5, 2024 23:24 View deployment

phipsae requested changes Dec 7, 2024

View reviewed changes

Abidoyesimze added 2 commits December 11, 2024 21:10

update the changes

3125de9

update

c3d0dd8

vercel bot deployed to Preview December 12, 2024 00:44 View deployment

phipsae requested changes Dec 12, 2024

View reviewed changes

phipsae assigned Abidoyesimze Dec 12, 2024

update the contract and the readme files

39ebc79

melanke reviewed Dec 14, 2024

View reviewed changes

phipsae requested changes Dec 15, 2024

View reviewed changes

Abidoyesimze and others added 4 commits December 16, 2024 03:03

update the deploy scripts

890ec2f

remove argument for deploying pixelCanva in deployment script

be9b9c8

reset yarn.lock file

ac16165

add PixelCanvas contract abi to externalContracts

0fad4a1

phipsae reviewed Dec 17, 2024

View reviewed changes

phipsae merged commit 270c25a into BuidlGuidl:main Dec 17, 2024
1 of 2 checks passed

vercel bot deployed to Preview December 17, 2024 10:22 View deployment

	Test the CanvasOnchain contract
	Test the PixelCanvas contract

Change the branch name to canvasOnchain and also write a readme file in the hardhat folder for description #32

Change the branch name to canvasOnchain and also write a readme file in the hardhat folder for description #32

Conversation

Abidoyesimze commented Dec 5, 2024 • edited Loading

Description

Additional Information

Related Issues

vercel bot commented Dec 5, 2024 • edited Loading

vercel bot commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melanke commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phipsae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phipsae left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phipsae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phipsae left a comment • edited Loading

Choose a reason for hiding this comment

Abidoyesimze commented Dec 5, 2024 •

edited

Loading

vercel bot commented Dec 5, 2024 •

edited

Loading

phipsae left a comment •

edited

Loading

phipsae left a comment •

edited

Loading