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

ref: Privatize and Rename State Fields #500

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

Conversation

0xNeshi
Copy link
Collaborator

@0xNeshi 0xNeshi commented Jan 17, 2025

Resolves #447, resolves #386

PR Checklist

  • Tests
  • Documentation
  • Changelog

@0xNeshi 0xNeshi self-assigned this Jan 17, 2025
Copy link

netlify bot commented Jan 17, 2025

Deploy Preview for contracts-stylus canceled.

Name Link
🔨 Latest commit ee4302d
🔍 Latest deploy log https://app.netlify.com/sites/contracts-stylus/deploys/679244950a90ed0008e96399

Copy link

codecov bot commented Jan 17, 2025

@0xNeshi 0xNeshi changed the title Renaming contr params ref: Privatize and Rename State Fields Jan 17, 2025
@0xNeshi 0xNeshi marked this pull request as ready for review January 17, 2025 10:55
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.

Looks much better!

@qalisander
Copy link
Member

qalisander commented Jan 21, 2025

Love the name of the pr:D May be postpone unit test wide refactors? This pr can have huge merge conflicts..

@0xNeshi
Copy link
Collaborator Author

0xNeshi commented Jan 21, 2025

May be postpone unit test wide refactors? This pr can have huge merge conflicts..

This would mean we either ignore all of the motsu tests (as they wouldn't compile), or that we completely close this PR 😕

We could remove field renames from this PR, and then open a new PR with those changes once your PR is merged.
@bidzyyys @qalisander how do you propose we approach this?

@bidzyyys
Copy link
Collaborator

IMHO we should merge this PR as it is finished. I do not believe we will have huge merge conflicts, only removing prefixes (_) from contract storage fields. I can handle solving this merge changes @qalisander.

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.

[Feature]: Make inner fields private for our contracts Rethink current naming convention in contracts
3 participants