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

FLIP for removing type requirements #118

Merged
merged 6 commits into from
Aug 15, 2023
Merged

Conversation

dsainati1
Copy link
Contributor

Adds a FLIP proposing to remove type requirements and change the semantics of event declarations in interfaces.

Part of onflow/cadence#1283

@dsainati1 dsainati1 force-pushed the sainati/remove-type-requirement branch from a175e1c to fb9bdff Compare July 11, 2023 16:37
@austinkline
Copy link
Contributor

Would it be possible to use NFT/FT definitions as examples and show how they'd change based on this FLIP?

@dsainati1
Copy link
Contributor Author

cc @joshuahannan are the contract rewrites for the FT/NFT contracts in a state where we can use them for this FLIP?

@joshuahannan
Copy link
Member

The new versions of the FT and NFT standards already don't use nested type requirements so you can take a look at those:

I'd like to merge those with the stable cadence feature branches so we can get them integrated into the preview release also, but just haven't been able to get started on it yet since I'm working on the core contracts right now

@bluesign
Copy link
Collaborator

I was thinking if I put events to an interface; I will control all events flowing from there.

As we turn into lets say single FT.Withdrawal event instead of FlowToken.Withdrawal; I can emit the event with (type: Type, address: Address, amount: UFix64) for example, people can listen a single event. ( very basic example )

So allowing others to emit would break the system.

@dsainati1
Copy link
Contributor Author

Practically speaking what @bluesign is suggesting would mean that when an event is declared inside an interface, it would only be available inside that interface's definition. E.g. if I had some event

contract interface I {
   event Foo()
}

Then Foo would only be available inside of I. Effectively it would be a private type definition. Would it be reasonable then to require it to be declared with access(self) to reflect this?

@bluesign
Copy link
Collaborator

Would it be reasonable then to require it to be declared with access(self) to reflect this?

I think emit already limits to defined contract scope when called. I think access(self) is not necessary in this case.

@SupunS
Copy link
Member

SupunS commented Jul 13, 2023

That's an interesting point. I think previously the requirement of declaring events as pub/access(all) was to allow implementations to have access to it (especially in interfaces, for type requirements). But now that requirement is gone, maybe mandating events to be defined as non-public is a good move?

That way, there won't be a need to keep this special casing of emit statements (to not allow emitting out-of-scope events) as well. The usual access modifiers would handle it.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

👍

cadence/20230711-remove-type-requirements.md Outdated Show resolved Hide resolved
cadence/20230711-remove-type-requirements.md Outdated Show resolved Hide resolved
However, note that this concrete type is actually defined like an interface, and includes no implementation.
Instead, this definiton of `Nested` requires any contracts implementing `Outer` to provide a definition of `Nested`
that conforms to the specification provided in `Outer`. This is called a nested type requirement.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe also explain how ExampleOuter implements Outer, so provides a concrete implementation of Outer.Nested as ExampleOuter.Nested

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

Feeling good about this. The token standards don't need to be able to define events in the interface anymore since the contract is a concrete type now instead of an interface

@dsainati1 dsainati1 merged commit 210a0df into main Aug 15, 2023
@dsainati1 dsainati1 deleted the sainati/remove-type-requirement branch August 15, 2023 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants