Migrate Upgrades to component #792
Conversation
| impl InternalImpl< | ||
| TContractState, +HasComponent<TContractState> | ||
| > of InternalTrait<TContractState> { | ||
| fn _upgrade(ref self: ComponentState<TContractState>, new_class_hash: ClassHash) { |
There was a problem hiding this comment.
do we still need the underscore?
There was a problem hiding this comment.
Andrew made a good point, we should define if we need it or not first, right?
There was a problem hiding this comment.
let's keep it for now and have that discussion then, did we have an issue for this?
| OwnableEvent: ownable_component::Event, | ||
| UpgradeableEvent: upgradeable_component::Event |
There was a problem hiding this comment.
should we be flattening events?
There was a problem hiding this comment.
My impression was that we need this only for ERC20 and ERC721 (backward compatibility with indexers). Not sure whether we should flatten all events.
There was a problem hiding this comment.
why would we want nested events? i tonight not flattening was just to allow "overriding" events, not to expose them nested. that'd be inconsistent, and make tooling/interop more complex
There was a problem hiding this comment.
any pre existing event should be flattened for backwards compat, this includes far more than erc20 and 721, also pausable, ownable, etc
There was a problem hiding this comment.
why would we want nested events? i tonight not flattening was just to allow "overriding" events, not to expose them nested. that'd be inconsistent, and make tooling/interop more complex
I think the purpose is to avoid potential clashes among components (the design from the cairo team)
There was a problem hiding this comment.
any pre existing event should be flattened for backwards compat, this includes far more than erc20 and 721, also pausable, ownable, etc
That's a fair point, but I still think tokens are probably more important than pausable on this matter
There was a problem hiding this comment.
I think the purpose is to avoid potential clashes among components
that makes sense when auto embedding, as a default mechanism. but it's an app-level decision whether to prefix events or not, and there's no standard for it. if anything, standards assume they're flattened.
That's a fair point, but I still think tokens are probably more important than pausable on this matter
what do you mean by "more important"? not sure what importance implies in this context, but I think we should be compatible and standard (although there's no written standard in Cairoland, there's Ethereum standards like Ownable and defacto standards such as Pausable) independently of importance i.e. even if ERC721 is "more important" than ERC20, we want both to be standard and compatible with existing tooling.
that's for existing standards, we yet need to decide what to do with new ones. this requires a bit more of standard discussion which might exceed this library's implementations.
Fixes #791
PR Checklist