Add ERC721ABI, fix other ABIs
#741
Conversation
There was a problem hiding this comment.
interfaces look good, are complete, and matching. but I thought the purpose of ABI traits was to offer a contract's (preset) full interface. we don't have a camel and a snake preset, we have a single preset using both interfaces (following the interface migration plan) and there should be a single ABI covering that. once we have more presets, each of them should have their own complete ABI.
as a side note, function order is inconsistent right now. I'd normalize function order between:
impl- ABI trait
- docs
All of the above applies to other presets as well, not just 721
|
This PR now includes the consolidated ABIs (with camelCase) for ERC20, ERC721, and Account. The ABIs include comments denoting the standard and non-standard whence they derive. The purpose of these comments is to:
Let me know if you have objections to the function order |
|
|
||
| * xref:#Account-\\__validate_deploy__[`++__validate_deploy__(self, hash, signature)++`] |
There was a problem hiding this comment.
shouldn't validate_declare be here too?
| [#Account-Utilities] | ||
| ==== Utilities | ||
|
|
There was a problem hiding this comment.
not sure if this is introduced in this PR or it comes from another, but i think we agreed on not having an Utilities section (not sure where, but maybe in the access PR)
| impl SRC6CamelOnlyImpl of interface::ISRC6CamelOnly<ContractState> { | ||
| fn isValidSignature( | ||
| self: @ContractState, hash: felt252, signature: Array<felt252> | ||
| ) -> felt252 { | ||
| SRC6Impl::is_valid_signature(self, hash, signature) | ||
| } | ||
| } | ||
|
|
||
| #[external(v0)] | ||
| impl SRC5CamelImpl of ISRC5Camel<ContractState> { |
There was a problem hiding this comment.
hm why one is SRC6CamelOnlyImpl and the other SRC5CamelImpl? we should normalize these names
There was a problem hiding this comment.
We are using CamelOnly when methods some are not added (having single-word function names), and just Camel, when all the methods are added (with the different casing).
| // ISRC5 | ||
| fn supports_interface(self: @TState, interface_id: felt252) -> bool; | ||
|
|
||
| // IDeclarer |
There was a problem hiding this comment.
is IDeclarer defined anywhere? if not let's put it with __validate_deploy__ as non-standard, I think it can be confusing otherwise.
There was a problem hiding this comment.
It's defined just above on line 24. I see your point though since it is non-standard. I'll move it to Non-standard to keep this simple
There was a problem hiding this comment.
I'll move it to Non-standard to keep this simple
Actually, I take it back. I think there's value in identifying non-standard extensions by their interface when applicable. I vote we keep it
| ) -> felt252; | ||
| fn setPublicKey(ref self: TState, newPublicKey: felt252); | ||
| fn getPublicKey(self: @TState) -> felt252; | ||
| // Account Camel |
There was a problem hiding this comment.
| // Account Camel | |
| // Camel case compatibility |
not sure if it's worth adding a link to the interface migration docs. let's propagate whatever we decide to the equivalent comments in ERC20 and 721
There was a problem hiding this comment.
It's probably not a bad idea to include a link. I'll add it and see what we think
Fixes #739.