ADR-003: Dynamic Capabilities #5828
Conversation
| @@ -326,6 +327,25 @@ func (key *TransientStoreKey) String() string { | |||
| return fmt.Sprintf("TransientStoreKey{%p, %s}", key, key.name) | |||
| } | |||
|
|
|||
| // MemoryStoreKey defines a typed key to be used with an in-memory KVStore. | |||
| type MemoryStoreKey struct { | |||
There was a problem hiding this comment.
I'd add a Type() sdk.StoreKeyType method to the sdk.StoreKey interface
| return Keeper{ | ||
| cdc: cdc, | ||
| storeKey: storeKey, | ||
| memKey: memKey, |
There was a problem hiding this comment.
I'd panic here if the memKey.Type() doesn't match sdk. MemoryStoreKey
There was a problem hiding this comment.
We could, but the app has to call InitializeAndSeal which does the check without us having to change the interface of StoreKey.
alexanderbez
requested review from
alessio,
hschoenburg and
nylira
as code owners
|
|
||
| // Set attempts to add a given owner to the CapabilityOwners. If the owner already | ||
| // exists, an error will be returned. | ||
| func (co *CapabilityOwners) Set(owner Owner) error { |
There was a problem hiding this comment.
Just a note, this is O(n), it could be O(1) if we use a map (should be safe since iteration order won't change behaviour), or O(log n) if we do a binary search to check for duplicates & then insert.
There was a problem hiding this comment.
Yes, I've already thought about this. But since the CapabilityOwners type is encoded and persisted to state, you cannot use a map. Even if you did use a map at runtime and use a slice when you persist, you'd still have O(n) conversion. I've thought of various ways to do this like keeping an ephemeral map/cache, but it doesn't seem clean.
However, binary search would work and we can cut down Set to O(log n). I'll address that.
There was a problem hiding this comment.
Updated. Note, worst case can still be O(n). Maybe this is overkill, idk. We could also define an initial capacity in the owner slice.
There was a problem hiding this comment.
I think it's fine for the time being.
Co-Authored-By: Aditya <adityasripal@gmail.com>
There was a problem hiding this comment.
Great job @alexanderbez, looks almost entirely correct to me, a few changes to documentation needed (probably copied from a slightly out-of-date spec) and a few minor comments, but overall nearly there. 🎉
|
Think the ADR still has the outdated behavior in e77496f#diff-b232b32a8f540d33f5040df6477179b1R175 |
Yes - we should delete the last sentence. |
|
|
||
| // Set attempts to add a given owner to the CapabilityOwners. If the owner already | ||
| // exists, an error will be returned. | ||
| func (co *CapabilityOwners) Set(owner Owner) error { |
There was a problem hiding this comment.
I think it's fine for the time being.
There was a problem hiding this comment.
utACK, good work @alexanderbez! looking forward to integrating this 👍
Description
Implementation of ADR 003 - Dynamic Capabilities.
ref: #5542
/cc @jackzampolin @cwgoes @AdityaSripal @fedekunze
For contributor use:
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerFor admin use:
WIP,R4R,docs, etc)