-
Notifications
You must be signed in to change notification settings - Fork 327
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
Migrate SRC5 to component #767
Changes from 107 commits
331844d
b9ec20b
bb40124
a0c3157
3a6e20d
96431de
7b38706
2fa00dd
f0036e7
8d781a8
ae37b1c
da57a91
fd42fdd
1cb1252
f0330cd
abc737a
4ca2620
8dd8ed3
091a29c
1c866f2
3405063
8fe3c82
ad928bc
d883eb5
543501b
2ecba91
6c3978b
60365c8
be1e9df
c20fd02
bce2506
35ede09
e55889a
9d5079c
ae89fa5
a54e98c
91dcad3
f48da2c
d671d4c
e86bcd2
42a160f
598c232
b2624ee
4a4bca9
0962051
a1950ee
c29b45c
e48a682
5f86f65
897facb
372de37
d86eb73
62688e7
ba69328
8534ec6
bc90bd5
c8fa9f9
3a8e3e7
6a1f729
bbcb875
5a27baa
ba29133
b44f7dc
33dbd56
65d03ae
a863859
c4590b9
f18fcdb
dc4a233
27514b1
8e10821
a00a9d3
0b40485
4e64434
79fe2db
177ad63
495ed8a
c28c758
f9c30da
457b98b
544ddac
4473243
e95def8
77089c1
3e08fa6
8307644
cece32b
32f9974
b40c35b
fbdd759
05429e4
adac09f
90be39f
fcdd6d1
7373daa
cbb5bf5
4e388f5
a1b559c
74eb4e8
420f320
715e473
f1241b5
9a274be
4d116ad
f0d8c6f
cb5b828
09c47ad
ec68494
8870cfe
748e725
4d86b18
fef31c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,10 +50,10 @@ on how to compute this ID. | |
use openzeppelin::introspection::src5::SRC5; | ||
``` | ||
|
||
SRC5 contract implementation extending xref:ISRC5[`ISRC5`]. | ||
SRC5 component extending xref:ISRC5[`ISRC5`]. | ||
|
||
[.contract-index] | ||
.External Functions | ||
.Embeddable Implementations | ||
-- | ||
.SRC5Impl | ||
|
||
|
@@ -69,8 +69,8 @@ SRC5 contract implementation extending xref:ISRC5[`ISRC5`]. | |
* xref:#SRC5-deregister_interface[`++deregister_interface(self, interface_id)++`] | ||
-- | ||
|
||
[#SRC5-External-Functions] | ||
==== External Functions | ||
[#SRC5-Embeddable-Implementations-Functions] | ||
==== Embeddable Implementations Functions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we change the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about leaving just ComponentState? I think the generic TState is implicit since TContractState is always required, and this will appear frequently in the APIs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call, and it's much less noisy. +1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I think we should leave just
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The impl we embed is not the one we write in the component, but the one the compiler generates from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm if we go that route, should we include a NOTE or something to explain the rationale behind using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree it can be confusing, but where can we add such note? It seems we would need to add it to every component API There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Repeating the same note is definitely not ideal. Even though this is about reading the API, it's IMO more about understanding the component architecture (as you described above). Maybe this can be addressed in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, let's stick with ContractState for embeddable impls functions, and ComponentState for InternalImpl in components, at least until we revisit how we present this in the Extensibility PR. wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding the title: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's call it just Embeddable Functions in the meantime (I think it reads better). Any suggestion is more than welcome. |
||
|
||
[.contract-item] | ||
[[SRC5-supports_interface]] | ||
|
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.
In the Ownable API, the index name is
Embeddable Implementations Functions
. We should normalizeThere 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.
Let's stick with just Embeddable Implementations then if you agree.