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.

Sign up for GitHub

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

Jump to bottom

docs: make Rust docs consistent #497

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

docs: make Rust docs consistent #497

wants to merge 18 commits into from

Conversation

DarkLord017
Copy link
Contributor

@DarkLord017 DarkLord017 commented Jan 15, 2025 •
edited
Loading

Resolves #476

PR Checklist

  • Tests
  • Documentation
  • Changelog

@netlify Netlify
Copy link

netlify bot commented Jan 15, 2025 •
edited
Loading

✅ Deploy Preview for contracts-stylus canceled.

Name Link
🔨 Latest commit c297553
🔍 Latest deploy log https://app.netlify.com/sites/contracts-stylus/deploys/679234c6044a6f00081722fa

@DarkLord017
Copy link
Contributor Author

I also added * mark befor each event
also i have kept avery panic after events so is that ok ?

@0xNeshi
Copy link
Collaborator

0xNeshi commented Jan 17, 2025

@DarkLord017 you should also fix the failing CI

@DarkLord017
Copy link
Contributor Author

Okay

1 similar comment
@DarkLord017
Copy link
Contributor Author

Okay

@DarkLord017
Copy link
Contributor Author

@DarkLord017 you should also fix the failing CI

You can review it now

0xNeshi
0xNeshi reviewed Jan 20, 2025
View reviewed changes
contracts/src/access/control.rs Outdated Show resolved Hide resolved
contracts/src/access/control.rs Outdated Show resolved Hide resolved
@bidzyyys bidzyyys changed the title Consistent docs docs: make Rust docs consistent Jan 20, 2025
@DarkLord017 DarkLord017 requested a review from 0xNeshi January 21, 2025 11:04
bidzyyys
bidzyyys reviewed Jan 21, 2025
View reviewed changes
Copy link
Collaborator

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

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

Great job @DarkLord017.

@bidzyyys bidzyyys self-requested a review January 21, 2025 13:32
bidzyyys
bidzyyys approved these changes Jan 21, 2025
View reviewed changes
Copy link
Collaborator

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

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

LGTM

contracts/src/token/erc20/utils/safe_erc20.rs Outdated Show resolved Hide resolved
@DarkLord017
Copy link
Contributor Author

Great job @DarkLord017.

Thank you !

0xNeshi
0xNeshi reviewed Jan 22, 2025
View reviewed changes
contracts/src/token/erc1155/extensions/supply.rs
Comment on lines +199 to +201
/// NOTE: The ERC-1155 acceptance check is not performed in this function.
/// See [`Self::_update_with_acceptance_check`] instead.
///
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

0xNeshi
0xNeshi reviewed Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@0xNeshi 0xNeshi left a comment

Choose a reason for hiding this comment

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

You did a great job @DarkLord017 , most of the comments I left are related to fine-tuning the details of the original issue requirements.

Sometimes we notice the changes we want to make only once the PR is created.

contracts/src/token/erc1155/extensions/uri_storage.rs Outdated Show resolved Hide resolved
contracts/src/token/erc1155/mod.rs
///
/// # Panics
///
/// Should not panic.
/// * Should not panic.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// * Should not panic.
/// Should not panic.

nit: there is nothing to list.

contracts/src/token/erc1155/mod.rs
Comment on lines 328 to +330
/// # Panics
///
/// Should not panic.
/// * Should not panic.
Copy link
Collaborator

@0xNeshi 0xNeshi Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@bidzyyys @qalisander should we even have the # Panics section for non-panicking functions?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be pointless. Make sense to remove it

contracts/src/token/erc1155/extensions/supply.rs Outdated Show resolved Hide resolved
contracts/src/token/erc1155/mod.rs Show resolved Hide resolved
contracts/src/token/erc721/extensions/uri_storage.rs Outdated Show resolved Hide resolved
contracts/src/utils/cryptography/ecdsa.rs Outdated Show resolved Hide resolved
contracts/src/utils/introspection/erc165.rs Outdated Show resolved Hide resolved
contracts/src/utils/introspection/erc165.rs Outdated Show resolved Hide resolved
contracts/src/utils/introspection/erc165.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bidzyyys bidzyyys bidzyyys approved these changes

@0xNeshi 0xNeshi 0xNeshi left review comments

@qalisander qalisander Awaiting requested review from qalisander qalisander is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

@DarkLord017 DarkLord017

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Docs]: Make Rust docs consistent
4 participants
@DarkLord017 @0xNeshi @bidzyyys @qalisander