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

build: bump motsu v0.4.0 #39

Merged
merged 25 commits into from
Feb 11, 2025
Merged

build: bump motsu v0.4.0 #39

merged 25 commits into from
Feb 11, 2025

Conversation

qalisander
Copy link
Member

@qalisander qalisander commented Feb 6, 2025

PR Checklist

  • Docs
  • Version
  • Changelog

README.md Show resolved Hide resolved
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.

You have a warning to solve:

Check warning on line 265 in crates/motsu/src/lib.rs

unused variable: `pong`

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.

Left some comments.

crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/src/lib.rs Outdated Show resolved Hide resolved
crates/motsu/src/lib.rs Outdated Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/src/lib.rs Show resolved Hide resolved
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.

I'm not in favor of using contracts from our unit tests in this README, as I find those contracts too unclear to be used to explain how Motsu works. A contract used for docs should be simple, yet meaningful, and Proxy is not really meaningful.
A good example of simple and meaningful tests would be the ping_pong_tests. But I'd advise against using even these tests as part of our docs, as they're not versatile enough (e.g. they don't use msg::value).

Ideally, we would define one or two contracts separate from unit tests, place those at the top of README(s), and then reuse these contracts for all subsequent code examples. This would both make it easier for readers to follow and understand our explanations, while making it unnecessary to specify imports in each example. I think neither ERC20 nor Proxy nor Ping/Pong are good candidates for this, and new contracts are necessary.
Let me know if you'd like me to suggest contracts for the docs, and I can take some time to think of good examples.

EDIT: if we updated Ping/Pong contracts to accept ether and pass it to each other, it would make them more valid examples for use in docs, as it would make sense to use .sender_and_value(...).

Alternatively, if the docs aren't high priority for this PR, we can keep using current contracts in READMEs, but create a separate issue to update our docs for some future release.

Wdyt @bidzyyys @qalisander ?

README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/src/lib.rs Outdated Show resolved Hide resolved
@0xNeshi 0xNeshi self-requested a review February 7, 2025 10:45
@bidzyyys
Copy link
Collaborator

bidzyyys commented Feb 7, 2025

I'm not in favor of using contracts from our unit tests in this README, as I find those contracts too unclear to be used to explain how Motsu works. A contract used for docs should be simple, yet meaningful, and Proxy is not really meaningful. A good example of simple and meaningful tests would be the ping_pong_tests. But I'd advise against using even these tests as part of our docs, as they're not versatile enough (e.g. they don't use msg::value).

Ideally, we would define one or two contracts separate from unit tests, place those at the top of README(s), and then reuse these contracts for all subsequent code examples. This would both make it easier for readers to follow and understand our explanations, while making it unnecessary to specify imports in each example. I think neither ERC20 nor Proxy nor Ping/Pong are good candidates for this, and new contracts are necessary. Let me know if you'd like me to suggest contracts for the docs, and I can take some time to think of good examples.

Alternatively, if the docs aren't high priority for this PR, we can keep using current contracts in READMEs, but create a separate issue to update our docs for some future release.

Wdyt @bidzyyys @qalisander ?

I believe we can take Counter example, as I believe it is auto generated by default from cargo stylus new command.

@0xNeshi
Copy link
Collaborator

0xNeshi commented Feb 7, 2025

I believe we can take Counter example, as I believe it is auto generated by default from cargo stylus new command

Counter has the same problem as Ping/Pong - it doesn't use msg::value, though Ping/Pong could be updated to do so, and then we could reuse this in docs. I'll update my original comment with a suggestion

@qalisander qalisander requested a review from bidzyyys February 7, 2025 17:30
README.md Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
crates/motsu/README.md Outdated Show resolved Hide resolved
crates/motsu/src/lib.rs Outdated Show resolved Hide resolved
@qalisander qalisander requested a review from 0xNeshi February 10, 2025 15:15
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.

Nice work!

@0xNeshi
Copy link
Collaborator

0xNeshi commented Feb 11, 2025

The fund-related change request left unresolved #39 (comment)

@qalisander
Copy link
Member Author

Update changelog date

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!

@qalisander qalisander merged commit 862770f into main Feb 11, 2025
10 checks passed
@bidzyyys bidzyyys deleted the build/motsu-v0.4.0 branch February 11, 2025 09:46
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