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

wording clarifications #20

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

wording clarifications #20

wants to merge 2 commits into from

Conversation

anikaraghu
Copy link
Contributor

Mostly nits I noticed while reading through that can help make it easier to read

README.md Outdated
#### 3. Events

##### A. Events names should be past tense.
#### 3. Events names should be past tense.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we were getting to have too many levels which made it hard to follow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could also instead link some sections out to different READMEs as an alternative

Copy link
Contributor

Choose a reason for hiding this comment

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

imo grouping is helpful and separate readmes would require more work, but we have talked about making a doc site. Agree we are pushing a single thing to the limit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm I guess it's a bit confusing that some are grouped and some aren't. Should we group all then?

or
If a function should never be called from another contract, it should be marked private and its name should have a leading underscore.

#### Justification
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have Justification elsewhere where we explain reasoning, so I am hesitant to mix up the format. (I used to have something like this but removed for consistency)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think exceptions should require justification. This is the only exception we've listed so I would expect future ones to have justification as well, but might not be present right now

README.md Outdated

```solidity
using Library for bytes
bytes._function()
```

Note, we cannot remedy this by insisting on the use public functions. Whether a library functions are internal or external has important implications. From the [Solidity documentation](https://docs.soliditylang.org/en/latest/contracts.html#libraries)
We could remedy this by insisting on the use public functions. However, developers may prefer internal functions because they are more gas efficient to call, due to how libraries are compiled in Solidity:
Copy link
Contributor

Choose a reason for hiding this comment

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

imo is confusing to say "we could" remedy. Maybe like "We should not remedy this by ..."

Copy link
Contributor

@wilsoncusack wilsoncusack left a comment

Choose a reason for hiding this comment

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

Thanks for this! There's significant formatting changes mixed in with wording changes here that makes this hard to get in together.

@anikaraghu anikaraghu force-pushed the anika/edits branch 2 times, most recently from 957bcb9 to f77a08b Compare May 2, 2024 15:02
@cb-heimdall
Copy link
Collaborator

Review Error for xenoliss @ 2024-05-08 12:03:46 UTC
User failed mfa authentication, see go/mfa-help

@anikaraghu anikaraghu force-pushed the anika/edits branch 3 times, most recently from ec2ed93 to 0b8a579 Compare May 14, 2024 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants