For #7, implement simple Data Object Type registry #13
Conversation
|
A pull requested should go into the |
is to call the module file `type_registry`, and the use the full `DataObjectTypeRegistry` name for the Module<T>.
yet for unit tests.
|
Ok, resolved conflicts and changed everything according to feedback. Looks good, no? |
| use system::{self, ensure_root}; | ||
| use crate::traits; | ||
|
|
||
| pub trait Trait: system::Trait + MaybeDebug |
There was a problem hiding this comment.
In Rust there is a convention to put opening { on the same line with a preceding code:
| pub trait Trait: system::Trait + MaybeDebug | |
| pub trait Trait: system::Trait + MaybeDebug { |
There was a problem hiding this comment.
I'm fine with changing that, but you're likely going to see this a lot from me :)
| pub enum Event<T> where | ||
| <T as Trait>::DataObjectTypeID | ||
| { | ||
| DataObjectTypeAdded(DataObjectTypeID), |
There was a problem hiding this comment.
Maybe Registered instead of Added? Such as a function is register_data_object_type
| DataObjectTypeAdded(DataObjectTypeID), | |
| DataObjectTypeRegistered(DataObjectTypeID), |
There was a problem hiding this comment.
Makes sense, will change.
| { | ||
| type Event: From<Event<Self>> + Into<<Self as system::Trait>::Event>; | ||
|
|
||
| type DataObjectTypeID: Parameter + Member + SimpleArithmetic + Codec + Default + Copy |
There was a problem hiding this comment.
By following Substrate and our naming convention for IDs, it should be: ID -> Id
| type DataObjectTypeID: Parameter + Member + SimpleArithmetic + Codec + Default + Copy | |
| type DataObjectTypeId: Parameter + Member + SimpleArithmetic + Codec + Default + Copy |
There was a problem hiding this comment.
Yeah, that's a naming convention that I've seen a lot in Java, and it's pretty stupid. ID is not a word, it's an abbreviation, so CamelCasing it makes no sense at all. Still, I will change it.
There was a problem hiding this comment.
Here is a quote from ID article on Wiki:
ID, Id, id or I.D. may refer to:
- Identity document, any document used to verify a person's identity
- Identifier, a symbol which uniquely identifies an object or record
I believe that in our cases (AccounId, MemberId, ProposalId) an Id stands for Identifier (i.e. id_entifier. ID could be considered as an abbreviation only in case if we reffer to I_dentity D_ocument – that is not our case at all.
| pub struct DataObjectType<T: Trait> | ||
| { | ||
| // If the OT is registered, an ID must exist, otherwise it's a new OT. | ||
| pub id: Option<T::DataObjectTypeID>, |
There was a problem hiding this comment.
I would suggest changing id type to just T::DataObjectTypeId and creating an additional struct just for updates:
pub struct DataObjectTypeUpdate<T: Trait> {
description: Option<Vec<u8>>,
active: Option<bool>,
}
And in update_* function, update only those fields of stored value that are Some in DataObjectTypeUpdate
There was a problem hiding this comment.
Ok, why? I suppose it may keep transaction sizes down. I suppose that could be worth it, but is that the rationale?
There was a problem hiding this comment.
The tx size is certainly a benefit, but I think the rationale behind alex's suggestion is so that you wouldn't have to change the function signature in the future if you introduce new fields to theDataObjectType type. (I don't want to put words in his mouth but that is how I understood it when he recommended the same for the members module update_profile() method)
There was a problem hiding this comment.
I actually didn't look at the function signature (i assumed there was one parameter per field).. so my point doesn't actually make sense.
There was a problem hiding this comment.
Because object's id is used quite often (at least on UI: list stored objects, view details of stored objects, etc.) and having it as an Option will require additional check storedObj.id.isSome ? ...then : else... everywhere. Comparing to update – this operation is rate and exists only on Update Form.
rustfmt + update dependency
Last set of dispatchables
update Athens Live Milestones
range headers can't determine the file size. Since we're not streaming at the moment, we can safely return file sizes.
assimilate council genesis config in mocks to fix benchmarks tests
3 participants
Should be the majority of the DOT registry. Tests are missing at this point; I want to add them as well.