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

"closed" structs #4637

Open
chriseth opened this issue Aug 1, 2018 · 14 comments

Comments

@chriseth
Copy link
Contributor

commented Aug 1, 2018

This came out of #4636 - it might make sense to declared certain structs to be closed in the sense that any write access to members of the structs can only be performed inside a certain library. This way, auditing the behaviour of such structs can be limited to auditing the library itself. For a token contract, for example, we could have certain functions which modify the balances that have certain guarantees. If the balances cannot be modified anywhere else, it is sufficient to only check the library.

@chriseth chriseth added this to To discuss in Backlog (breaking) via automation Aug 1, 2018

@ekpyron

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2018

Sounds like a good idea. It will probably be possible to sneak around this restriction at the EVM/assembly level, but it will probably still be helpful.

@axic

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

Sounds good, I wonder if we should just add function members on structs for doing so? (No proposal yet.)

I guess adding function members is the C++ way and what we have now is kind of the Rust way.

@chriseth

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2018

So we would have something like

struct S semantics LibraryName { ... }

although we should find a better keyword there :)

@federicobond

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

+1 on the proposal. It would become an "opaque" data structure. right? I vote for overloading the keyword private for this use case.

Opaque structs should not allow instantiation via raw struct constructors. Any initialization should be done via an exposed library function.

@chriseth

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

@federicobond good point about instantiation - you are totally right! Although this might be difficult for storage structs and zero-initialization, especially for structs as mapping values.

As far as the syntax is concerned: Would you propose struct S private to LibraryName { ... }?

We could also use struct S internal in LibraryName { ... } or
struct S { ... } LibraryName implements S.

struct S implementedBy LibraryName { ... }?

@chriseth

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

Actually it would even make sense for this to implicitly enable using LibnaryName for StructType, so perhaps this could be an extension of the using directive and not the struct declaration - of course this only becomes useful once we allow using at the file level, but then it might be complicated to get the import semantics right....

@chriseth

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

So in the end, we should perhaps both have using and private to LibraryName as part of the struct type itself and not local to some scope.

@spalladino

This comment has been minimized.

Copy link

commented Aug 3, 2018

Maybe I'm missing something, but if the struct is to be instantiated and modified only inside a certain library, shouldn't it be defined in that library as well? So, instead of having a using Struct private to LibraryName, the struct is defined within the library and must be referred to with a qualified name when used. Something like:

library MyLibrary {
  struct MyStruct { ... }
}

contract MyContract {
  using MyLibrary;
  MyLibrary.MyStruct field;
}
@chriseth

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

@spalladino I think that is just very redundant.

@nventuro

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2018

This sounds like a very useful feature: see this comment for a recent instance were OpenZeppelin could've benefited from it.

@nventuro

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

If the whole purpose of the library is to provide methods to the struct, why give a name to both? I like @axic's suggestion of adding functions directly to the struct (and tagging struct members private if desired), though we'd then have the problem of importing it: afaik, the only allowed top-level constructs are libraries and contracts.

@ekpyron

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

#6920 in combination with this suggests that it might make sense to add some kind of notion of "inheritance for libraries" or "library interfaces", so that different libraries implementing a particular interface can access the struct (to satisfy both use cases it'd of course have to be possible to allow and disallow access via inheritance).

@nventuro

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@ekpyron are you suggesting something along the lines of what C++ does with public, protected and private? (brief description here)

@ekpyron

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@nventuro Yes, something like that - if we extend the proposal in this issue like that, we'd probably reasonably solve the request in #6920 as well using one and the same language concept. So far, however, we don't have library inheritance at all, so we need to decide whether it's worth to introduce it for this purpose.
EDIT: although I'm actually not sure whether the state transition contracts in #6920 should actually be libraries or whether it should be regular contracts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.