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

Update structs-exercise.md Data type warning #177

Merged
merged 3 commits into from
Dec 12, 2023
Merged

Conversation

RenierC
Copy link
Contributor

@RenierC RenierC commented Dec 5, 2023

What changed? Why?
Adding a new message at the end of the exercise to avoid future errors. (just a proposal we can change the copy if needed be).

Notes to reviewers
I've run into an error in the automated tests, specifically regarding the Car struct. The tests seem to expect a uint256 for the numberOfDoors declaration, causing a somewhat cryptic "wrong signature" error.

Given the exercises leading up to this point, it's intuitive to lean towards the smallest uint, like uint8 given that several fellow learners at #basecamp-chat on discord and myself have faced this, and I propose the warning in my Pr to address this.

How has it been tested?
Tried to submit a contract with uint8 in the struct, it gave a cryptic error and then I just changed it to uint256 and it passed.

I've run into an error in the automated tests, specifically regarding the Car struct. The tests seem to expect a `uint256` for the `numberOfDoors` declaration, causing a somewhat cryptic "wrong signature" error.

Given the exercises leading up to this point, it's intuitive to lean towards the smallest uint, like `uint8` given that several fellow learners at #basecamp-chat on discord and myself have faced this, and I propose the warning in my Pr to address this.
@wbnns
Copy link
Member

wbnns commented Dec 7, 2023

@RenierC

Thanks for taking the time to open this PR! Will leave this open until our team has an opportunity to review.

@wbnns wbnns added the state: backlog This is pending further review label Dec 7, 2023
Copy link
Contributor

@briandoyle81CB briandoyle81CB left a comment

Choose a reason for hiding this comment

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

Hi @RenierC, thanks for taking the time to write this!

The exercise is actually structured this way on purpose, so that the learner is tested to see if they understand when using a uint8 will have a gas advantage in packing, and when (such as in this case) a uint8 is the incorrect choice, since there is no packing and the VM has to cast it to 32 bytes every time it operates on it.

But given the recent conversations, I do agree that some tuning is necessary. Do you mind trying to think of a subtler hint to put in the Car Struct section?

@cb-heimdall
Copy link
Collaborator

Review Error for briandoyle81CB @ 2023-12-07 16:58:22 UTC
User failed mfa authentication, see go/mfa-help

Copy link
Contributor

@briandoyle81CB briandoyle81CB left a comment

Choose a reason for hiding this comment

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

Didn't do mfa, please see comment above

@RenierC
Copy link
Contributor Author

RenierC commented Dec 10, 2023

Hi @RenierC, thanks for taking the time to write this!

The exercise is actually structured this way on purpose, so that the learner is tested to see if they understand when using a uint8 will have a gas advantage in packing, and when (such as in this case) a uint8 is the incorrect choice, since there is no packing and the VM has to cast it to 32 bytes every time it operates on it.

But given the recent conversations, I do agree that some tuning is necessary. Do you mind trying to think of a subtler hint to put in the Car Struct section?

@briandoyle81CB You're welcome! and thx for clarifying this, I remember that when you hinted this at me on the discord I read 2 docs that pushed me in the right direction, I set up a spoiler at the bottom with those resources and made it more subtle in the new commits and removed the previous warning I made.

Closed spoiler

image

Open spoiler

image

The urls take you here https://docs.soliditylang.org/en/v0.8.17/internals/layout_in_storage.html#layout-of-state-variables-in-storage and here https://docs.base.org/base-camp/docs/structs/structs-sbs/#setting-up-the-struct

Let me know what you think and if I need to improve on this 😉.

@briandoyle81CB
Copy link
Contributor

Hi @RenierC, thanks for taking the time to write this!
The exercise is actually structured this way on purpose, so that the learner is tested to see if they understand when using a uint8 will have a gas advantage in packing, and when (such as in this case) a uint8 is the incorrect choice, since there is no packing and the VM has to cast it to 32 bytes every time it operates on it.
But given the recent conversations, I do agree that some tuning is necessary. Do you mind trying to think of a subtler hint to put in the Car Struct section?

@briandoyle81CB You're welcome! and thx for clarifying this, I remember that when you hinted this at me on the discord I read 2 docs that pushed me in the right direction, I set up a spoiler at the bottom with those resources and made it more subtle in the new commits and removed the previous warning I made.

Closed spoiler

image

Open spoiler

image

The urls take you here https://docs.soliditylang.org/en/v0.8.17/internals/layout_in_storage.html#layout-of-state-variables-in-storage and here https://docs.base.org/base-camp/docs/structs/structs-sbs/#setting-up-the-struct

Let me know what you think and if I need to improve on this 😉.

Perfect, thank you!

Copy link
Member

@wbnns wbnns left a comment

Choose a reason for hiding this comment

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

@RenierC

Thanks!

@wbnns wbnns merged commit 92bd6d1 into base-org:master Dec 12, 2023
3 checks passed
kirkas pushed a commit that referenced this pull request Oct 21, 2024
* Update structs-exercise.md Data type warning

I've run into an error in the automated tests, specifically regarding the Car struct. The tests seem to expect a `uint256` for the `numberOfDoors` declaration, causing a somewhat cryptic "wrong signature" error.

Given the exercises leading up to this point, it's intuitive to lean towards the smallest uint, like `uint8` given that several fellow learners at #basecamp-chat on discord and myself have faced this, and I propose the warning in my Pr to address this.

* adds spoiler for struct exercise
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: backlog This is pending further review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants