-
Notifications
You must be signed in to change notification settings - Fork 21
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
Minor style improvements in Law-Stone
Smart Contract
#608
Conversation
WalkthroughThe recent changes enhance the Rust codebase by implementing stricter compilation rules during testing and reorganizing the structure of the code for better readability. The addition of Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant Cargo
participant Codebase
TestRunner->>Cargo: Run Tests
Cargo->>Codebase: Check Code for Warnings
Codebase-->>Cargo: Return Warnings/Errors
alt Warnings Present
Cargo-->>TestRunner: Fail Tests
else No Warnings
Cargo-->>TestRunner: Pass Tests
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
69b84f8
to
a08e7cf
Compare
After deprecation of SubMsgResponse::data in cosmwasm v2.0
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
a08e7cf
to
eac95ad
Compare
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- Makefile.toml (1 hunks)
- contracts/axone-law-stone/src/contract.rs (10 hunks)
Additional comments not posted (8)
Makefile.toml (1)
60-60
: LGTM! Enforcing stricter compilation rules.The addition of
RUSTFLAGS = "-D warnings"
under the testing command ensures that warnings are treated as errors, promoting code quality.contracts/axone-law-stone/src/contract.rs (7)
10-14
: LGTM! Improved import organization.The rearrangement of import statements enhances code readability and maintainability.
69-72
: LGTM! Refactoredbreak_stone
function.The use of iterator methods such as
filter
andmap
improves the clarity and conciseness of the code.
133-141
: LGTM! Improvedask
function logic.The refactoring of the
ask
function enhances its logic and readability.
206-211
: LGTM! Streamlinedstore_program_reply
function.The refactoring of the
store_program_reply
function improves clarity and maintainability.
282-284
: LGTM! Improved test module imports organization.The reorganization of test module imports enhances code readability and maintainability.
705-705
: LGTM! Allowing deprecated usage in test case.The addition of
#[allow(deprecated)]
ensures that thestore_program_reply
test case remains functional despite deprecation warnings.
792-792
: LGTM! Allowing deprecated usage in test case.The addition of
#[allow(deprecated)]
ensures that theprogram_reply_errors
test case remains functional despite deprecation warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice ! It's a shame that Cosmwasm didn't offer a second interface without the data
attribute (and thus without the deprecated).
Haha, yeah. It’s a bit annoying, but nothing too problematic. I searched around and couldn’t find anything relevant, other than disabling the deprecation warning (as it is shown in the comments btw). I was hoping for a builder or such with default values so we could just skip the deprecated field. But hey, I don’t feel like bugging @webmaster128 about it. 😅 |
I see. This only affects testing code, right? Because in the production use case What we can do is to create a constructor |
I understand, it's not problematic at all. It's just a small remark to improve code quality by avoiding warnings and removing them manually. 😉
@webmaster128 Yes, as far as I saw, it's only in the testing code. Yes, adding constructor without the deprecated field is a good solution! But don't worry about it, I might contribute to the project and add it myself since I'm the one complaining. 😉 |
Minor style improvements in
Law-Stone
Smart Contract:mut
)Summary by CodeRabbit