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

Update extensibility pattern #303

Merged
merged 8 commits into from
May 13, 2022
Merged

Update extensibility pattern #303

merged 8 commits into from
May 13, 2022

Conversation

martriay
Copy link
Contributor

@martriay martriay commented May 6, 2022

Resolves #295. I think the bits about constructors show that we need to differentiate between a contract constructor and a "library constructor". I think this is confusing enough to rollback and call them initializers.

Comment on lines 34 to 36
- `A`: internal to a library, not meant to be used outside the module
- `B`: part of the public API of a library
- `C`: subset of B that is meant to be exported as-is by contracts
Copy link
Contributor Author

@martriay martriay May 6, 2022

Choose a reason for hiding this comment

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

should we call them private, internal, and public? I don't want to coin any term that might conflict with the language down the road, or mix programming concepts and confuse people.

Copy link
Contributor

Choose a reason for hiding this comment

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

my vote is to adopt private, internal, public. even if the safeguards aren't there yet this will still help developers coming from other languages wrap their heads around building new contracts and reading existing ones.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that those could definitely work^. I wonder if it'd be easier to use a more literal approach:
internal: internal to the lib...
public: part of the public API...
complete, finished, or something like that: ...meant to be exported as-is...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably external

Copy link
Contributor

Choose a reason for hiding this comment

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

^+1

Copy link
Contributor

Choose a reason for hiding this comment

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

external has a clash with @external - a @view func is also external

may I suggest final?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but it's not really final since they can still be extended, most notably ERC20.transfer

Copy link
Contributor

Choose a reason for hiding this comment

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

External makes sense to me. The clash with @external seems benign given that external functions should generally be wrapped in an @external function, no?

Copy link
Collaborator

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

@martriay martriay merged commit 7f15120 into main May 13, 2022
@martriay martriay deleted the update-extensibility branch May 13, 2022 22: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.

Update extensibility pattern documentation
5 participants