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

create the CanvasOnchain contract and write the test #31

Closed
wants to merge 2 commits into from

Conversation

Abidoyesimze
Copy link
Contributor

@Abidoyesimze Abidoyesimze commented Dec 4, 2024

Description

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

Additional Information

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:

Copy link

vercel bot commented Dec 4, 2024

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

A member of the Team first needs to authorize it.

Copy link
Contributor

@melanke melanke left a comment

Choose a reason for hiding this comment

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

Very Nice! I am impressed on how good this is! I love the tests! :)

I've just added a discussion on a subject, I am not saying what you did is incorrect, just a concern.

Comment on lines 112 to 122
/**
* @dev Reset a specific pixel (only owner)
* @param x X-coordinate of the pixel
* @param y Y-coordinate of the pixel
*/
function resetPixel(uint256 x, uint256 y) external onlyOwner {
require(x < CANVAS_WIDTH, "X coordinate out of bounds");
require(y < CANVAS_HEIGHT, "Y coordinate out of bounds");

delete canvas[x][y];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little bit authoritary, don't you think? Why the owner has the rights to erase a pixel?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, we could override a pixel anyway, but, why erase? And without an event...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay will make changes for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a little bit authoritary, don't you think? Why the owner has the rights to erase a pixel?

Well, that's true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we could override a pixel anyway, but, why erase? And without an event...

I have remove the reset pixel functionality

Copy link
Contributor

@melanke melanke left a comment

Choose a reason for hiding this comment

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

We need the deployment script, right?

@phipsae
Copy link
Collaborator

phipsae commented Dec 5, 2024

@Abidoyesimze Nice, a smart contract, thanks!

Please ensure you include a description for your PR. As a rule of thumb, always provide a proper description when creating a PR, it’s essential for collaboration.

Please give the branch always a dedicated name, what you want to implement. smize doesn't tell me anything and I think it was the same before for your personal page.

Sorry to stress that, but thats what we try to teach you here in the batch program. Pls fix this first, then I will have a look at your code.

And a deployment script would be nice. As soon as you have it deployed you can add the ABI to the externalContract.ts file in next.js. Then it is shown in the debug page.

@Abidoyesimze
Copy link
Contributor Author

We need the deployment script, right?

Ohhhh yes but i don't know if we are deploying it to optimism mainnet that's why i haven't deploy it yet

@Abidoyesimze
Copy link
Contributor Author

@Abidoyesimze Nice, a smart contract, thanks!

Please ensure you include a description for your PR. As a rule of thumb, always provide a proper description when creating a PR, it’s essential for collaboration.

Please give the branch always a dedicated name, what you want to implement. smize doesn't tell me anything and I think it was the same before for your personal page.

Sorry to stress that, but thats what we try to teach you here in the batch program. Pls fix this first, then I will have a look at your code.

And a deployment script would be nice. As soon as you have it deployed you can add the ABI to the externalContract.ts file in next.js. Then it is shown in the debug page.

Sure, i'll make changes to the branch name but are we deploying the contract to optimism mainnet

@Abidoyesimze Abidoyesimze deleted the simze branch December 5, 2024 04:19
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.

3 participants