Skip to content
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.

Sign up for GitHub

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

fix(platform)!: document types should not have a contested unique index with a unique index #1984

Merged
merged 11 commits into from
Jul 23, 2024
Merged

fix(platform)!: document types should not have a contested unique index with a unique index #1984

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
102f556
dashpay contract update
QuantumExplorer Jul 22, 2024
3a0a930
temp work
QuantumExplorer Jul 22, 2024
5a859c1
work done
QuantumExplorer Jul 22, 2024
1648b6d
Merge branch 'v1.0-dev' into fix/UniqueContestedResourceFix
QuantumExplorer Jul 22, 2024
9d89439
update
QuantumExplorer Jul 22, 2024
931eca8
final fixes
QuantumExplorer Jul 23, 2024
2b756c8
final fixes
QuantumExplorer Jul 23, 2024
35b51fe
fixed dpns tests
QuantumExplorer Jul 23, 2024
4b74d69
fix
QuantumExplorer Jul 23, 2024
8a25164
fix
QuantumExplorer Jul 23, 2024
f056a2d
comment removal
QuantumExplorer Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
33 changes: 6 additions & 27 deletions packages/dpns-contract/schema/v1/dpns-contract-documents.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,11 @@
}
},
{
"name": "dashIdentityId",
"name": "identityId",
"nullSearchable": false,
"properties": [
{
"records.dashUniqueIdentityId": "asc"
}
],
"unique": true
},
{
"name": "dashAlias",
"properties": [
{
"records.dashAliasIdentityId": "asc"
"records.identity": "asc"
}
]
}
Expand Down Expand Up @@ -91,32 +83,19 @@
"records": {
"type": "object",
"properties": {
"dashUniqueIdentityId": {
"type": "array",
"byteArray": true,
"minItems": 32,
"maxItems": 32,
"position": 0,
"contentMediaType": "application/x.dash.dpp.identifier",
"description": "Identity ID to be used to create the primary name the Identity",
"$comment": "Must be equal to the document owner"
},
"dashAliasIdentityId": {
"identity": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please call it dashIdentityId it means dash identity identifier. We use always postfix id if it's identifier (ownerId).
Postfix dash make sense because other networks also have identities (yes we are going to resolve addresses for other networks as well) but here we referencing to our dash identities

Copy link
Member Author

@QuantumExplorer QuantumExplorer Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave what chat-gpt-4o says on the matter here:

In this debate, both perspectives have merit, but let's break down the arguments based on naming conventions and industry practices:

Naming Conventions and Industry Practices

  1. Contextual Naming vs. Technical Naming:
    • Your Perspective (Contextual Naming): Naming fields based on the business logic and the end-use (e.g., identity instead of identityId) is often more user-friendly and aligns with practices in systems like DNS and ENS. In these systems, the names used are typically user-facing and meant to be intuitive, reflecting the business concept rather than the underlying technical detail.
    • Colleague's Perspective (Technical Naming): Using identityId provides clarity that this field holds an identifier, which can be beneficial for developers and systems interfacing with multiple types of identifiers. It explicitly states that the value is an identifier, which can reduce ambiguity.
  2. Namespace Prefixing:
    • Your Perspective (Non-Prefixing): Avoiding prefixes like DashIdentity aligns with practices in broader industry standards, promoting interoperability and avoiding unnecessary verbosity. This approach also helps in keeping records concise, which can reduce costs and complexity.
    • Colleague's Perspective (Prefixing): Prefixing with Dash might help in scenarios where there's a need to distinguish between different identity services or networks. It can be useful in multi-network environments to avoid confusion, especially if there are overlapping namespaces or similar constructs in different networks.

Common Practices in DNS and ENS

• DNS: In DNS, records are named based on their function and the resource they point to, such as A, AAAA, MX, TXT, etc. These names are intuitive and indicate the type of resource directly, without additional prefixes.
• ENS: Similar to DNS, ENS names records based on their purpose and the resource they represent, like address, contenthash, text, etc. This approach emphasizes ease of use and understanding for end-users and developers alike.

Evaluation

• User-Friendliness: Naming fields in a way that reflects their business purpose (identity) rather than their technical representation (identityId) aligns with practices in user-facing systems and can make the system more intuitive.
• Technical Clarity: Using identityId can provide clear technical context, especially in a multi-service environment, reducing potential ambiguities.

Conclusion

Both perspectives have valid points, but the preference often leans towards user-friendly, business logic-based naming in high-level schemas and interfaces, which aligns with industry standards seen in DNS and ENS. However, for lower-level or more technically detailed interfaces, including the type of data (like Id) can be beneficial for clarity.

Given that DPNS aims to be a user-facing service similar to DNS and ENS, your approach to use identity without the prefix or suffix seems more aligned with making the service intuitive and easy to use. Your colleague's point about being clear with identifiers is valid in a more technical or backend context, where such distinctions are critical. For high-level schemas intended for broader use, keeping names straightforward and business-oriented typically prevails.

"type": "array",
"byteArray": true,
"minItems": 32,
"maxItems": 32,
"position": 1,
"contentMediaType": "application/x.dash.dpp.identifier",
"description": "Identity ID to be used to create alias names for the Identity",
"$comment": "Must be equal to the document owner"
"description": "Identifier name record that refers to an Identity"
}
},
"minProperties": 1,
"maxProperties": 1,
"position": 5,
"additionalProperties": false,
"$comment": "Constraint with max and min properties ensure that only one identity record is used - either a `dashUniqueIdentityId` or a `dashAliasIdentityId`"
"additionalProperties": false
},
"subdomainRules": {
"type": "object",
Expand Down
3 changes: 3 additions & 0 deletions packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,9 @@
"unique": {
"type": "boolean"
},
"nullSearchable": {
"type": "boolean"
},
"contested": {
"type": "object",
"properties": {
Expand Down
41 changes: 41 additions & 0 deletions packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ use std::convert::TryInto;

#[cfg(feature = "validation")]
use crate::consensus::basic::data_contract::ContestedUniqueIndexOnMutableDocumentTypeError;
#[cfg(feature = "validation")]
use crate::consensus::basic::data_contract::ContestedUniqueIndexWithUniqueIndexError;
#[cfg(any(test, feature = "validation"))]
use crate::consensus::basic::data_contract::InvalidDocumentTypeNameError;
#[cfg(feature = "validation")]
Expand Down Expand Up @@ -50,6 +52,7 @@ use crate::version::PlatformVersion;
use crate::ProtocolError;
use platform_value::btreemap_extensions::BTreeValueMapHelper;
use platform_value::{Identifier, Value};

const NOT_ALLOWED_SYSTEM_PROPERTIES: [&str; 1] = ["$id"];

const SYSTEM_PROPERTIES: [&str; 11] = [
Expand Down Expand Up @@ -282,6 +285,12 @@ impl DocumentTypeV0 {
#[cfg(feature = "validation")]
let mut unique_indices_count = 0;

#[cfg(feature = "validation")]
let mut last_non_contested_unique_index_name: Option<String> = None;

#[cfg(feature = "validation")]
let mut last_contested_unique_index_name: Option<String> = None;

#[cfg(feature = "validation")]
let mut contested_indices_count = 0;

Expand Down Expand Up @@ -330,6 +339,23 @@ impl DocumentTypeV0 {
.into(),
)));
}

if let Some(last_contested_unique_index_name) =
last_contested_unique_index_name.as_ref()
{
return Err(ProtocolError::ConsensusError(Box::new(
ContestedUniqueIndexWithUniqueIndexError::new(
name.to_string(),
last_contested_unique_index_name.clone(),
index.name,
)
.into(),
)));
}

if index.contested_index.is_none() {
last_non_contested_unique_index_name = Some(index.name.clone());
}
}

if index.contested_index.is_some() {
Expand All @@ -355,6 +381,19 @@ impl DocumentTypeV0 {
)));
}

if let Some(last_unique_index_name) =
last_non_contested_unique_index_name.as_ref()
{
return Err(ProtocolError::ConsensusError(Box::new(
ContestedUniqueIndexWithUniqueIndexError::new(
name.to_string(),
index.name,
last_unique_index_name.clone(),
)
.into(),
)));
}

if documents_mutable {
return Err(ProtocolError::ConsensusError(Box::new(
ContestedUniqueIndexOnMutableDocumentTypeError::new(
Expand All @@ -364,6 +403,8 @@ impl DocumentTypeV0 {
.into(),
)));
}

last_contested_unique_index_name = Some(index.name.clone());
}

// Index names must be unique for the document type
Expand Down
12 changes: 12 additions & 0 deletions packages/rs-dpp/src/data_contract/document_type/index/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,22 +154,22 @@
}
}

impl PartialOrd for ContestedIndexFieldMatch {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
use ContestedIndexFieldMatch::*;
match (self, other) {
// Comparing two integers
(PositiveIntegerMatch(a), PositiveIntegerMatch(b)) => a.partial_cmp(b),

// Arbitrarily decide that any Regex is less than any PositiveIntegerMatch
(Regex(_), PositiveIntegerMatch(_)) => Some(Ordering::Less),
(PositiveIntegerMatch(_), Regex(_)) => Some(Ordering::Greater),

// Comparing Regex with Regex, perhaps based on pattern length
(Regex(a), Regex(b)) => a.as_str().len().partial_cmp(&b.as_str().len()),
}
}
}

Check warning on line 172 in packages/rs-dpp/src/data_contract/document_type/index/mod.rs

View workflow job for this annotation

GitHub Actions / Rust packages (dpp) / Linting

non-canonical implementation of `partial_cmp` on an `Ord` type 

warning: non-canonical implementation of `partial_cmp` on an `Ord` type
   --> packages/rs-dpp/src/data_contract/document_type/index/mod.rs:157:1
    |
157 | /  impl PartialOrd for ContestedIndexFieldMatch {
158 | |      fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
    | | _____________________________________________________________-
159 | ||         use ContestedIndexFieldMatch::*;
160 | ||         match (self, other) {
161 | ||             // Comparing two integers
...   ||
170 | ||         }
171 | ||     }
    | ||_____- help: change this to: `{ Some(self.cmp(other)) }`
172 | |  }
    | |__^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_partial_ord_impl
    = note: `#[warn(clippy::non_canonical_partial_ord_impl)]` on by default

impl Ord for ContestedIndexFieldMatch {
fn cmp(&self, other: &Self) -> Ordering {
Expand Down Expand Up @@ -261,6 +261,9 @@
pub name: String,
pub properties: Vec<IndexProperty>,
pub unique: bool,
/// Null searchable indicates what to do if all members of the index are null
/// If this is set to false then we do not insert references which makes such items non-searchable
pub null_searchable: bool,
/// Contested indexes are useful when a resource is considered valuable
pub contested_index: Option<ContestedIndexInformation>,
}
Expand Down Expand Up @@ -431,6 +434,9 @@
// For properties, we iterate each and move it to IndexProperty

let mut unique = false;
// The default for null searchable should be true. Do not change this without very
// careful thought and consideration.
let mut null_searchable = true;
let mut name = None;
let mut contested_index = None;
let mut index_properties: Vec<IndexProperty> = Vec::new();
Expand All @@ -454,6 +460,11 @@
unique = value_value.as_bool().expect("confirmed as bool");
}
}
"nullSearchable" => {
if value_value.is_bool() {
null_searchable = value_value.as_bool().expect("confirmed as bool");
}
}
"contested" => {
let contested_properties_value_map = value_value.to_map()?;

Expand Down Expand Up @@ -489,7 +500,7 @@
Regex::new(&regex).map_err(|e| {
RegexError(format!(
"invalid field match regex: {}",
e.to_string()

Check warning on line 503 in packages/rs-dpp/src/data_contract/document_type/index/mod.rs

View workflow job for this annotation

GitHub Actions / Rust packages (dpp) / Linting

`to_string` applied to a type that implements `Display` in `format!` args 

warning: `to_string` applied to a type that implements `Display` in `format!` args
   --> packages/rs-dpp/src/data_contract/document_type/index/mod.rs:503:66
    |
503 | ...                   e.to_string()
    |                        ^^^^^^^^^^^^ help: remove this
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_format_args
    = note: `#[warn(clippy::to_string_in_format_args)]` on by default
))
})?,
));
Expand Down Expand Up @@ -580,6 +591,7 @@
name,
properties: index_properties,
unique,
null_searchable,
contested_index,
})
}
Expand Down
1 change: 1 addition & 0 deletions packages/rs-dpp/src/data_contract/document_type/index/random_index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ impl Index {
name: index_name,
properties,
unique,
null_searchable: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to have it default you better give it opposite name so default value will be false (which is convention) and you use true to override it (to ignore nulls)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the opposite is very hard to understand: "ignore_null_searchable"? This is already hard enough.

contested_index: None,
})
}
Expand Down
32 changes: 29 additions & 3 deletions packages/rs-dpp/src/data_contract/document_type/index_level/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ pub enum IndexType {
ContestedResourceIndex,
}

#[derive(Debug, PartialEq, Copy, Clone)]
pub struct IndexLevelTypeInfo {
/// should we insert if all fields up to here are null
pub should_insert_with_all_null: bool,
/// The index type
pub index_type: IndexType,
}

impl IndexType {
pub fn is_unique(&self) -> bool {
match self {
Expand All @@ -39,12 +47,14 @@ impl IndexType {
}
}

pub type ShouldInsertWithAllNull = bool;

#[derive(Debug, PartialEq, Clone)]
pub struct IndexLevel {
/// the lower index levels from this level
sub_index_levels: BTreeMap<String, IndexLevel>,
/// did an index terminate at this level
has_index_with_type: Option<IndexType>,
has_index_with_type: Option<IndexLevelTypeInfo>,
/// unique level identifier
level_identifier: u64,
}
Expand All @@ -58,7 +68,7 @@ impl IndexLevel {
&self.sub_index_levels
}

pub fn has_index_with_type(&self) -> Option<IndexType> {
pub fn has_index_with_type(&self) -> Option<IndexLevelTypeInfo> {
self.has_index_with_type
}

Expand Down Expand Up @@ -199,7 +209,12 @@ impl IndexLevel {
NonUniqueIndex
};

current_level.has_index_with_type = Some(index_type);
// if things are null searchable that means we should insert with all null

current_level.has_index_with_type = Some(IndexLevelTypeInfo {
should_insert_with_all_null: index.null_searchable,
index_type,
});
}
}
}
Expand Down Expand Up @@ -265,6 +280,7 @@ mod tests {
ascending: false,
}],
unique: false,
null_searchable: true,
contested_index: None,
}];

Expand All @@ -291,6 +307,7 @@ mod tests {
ascending: false,
}],
unique: false,
null_searchable: true,
contested_index: None,
}];

Expand All @@ -302,6 +319,7 @@ mod tests {
ascending: false,
}],
unique: false,
null_searchable: true,
contested_index: None,
},
Index {
Expand All @@ -311,6 +329,7 @@ mod tests {
ascending: false,
}],
unique: false,
null_searchable: true,
contested_index: None,
},
];
Expand Down Expand Up @@ -346,6 +365,7 @@ mod tests {
ascending: false,
}],
unique: false,
null_searchable: true,
contested_index: None,
},
Index {
Expand All @@ -355,6 +375,7 @@ mod tests {
ascending: false,
}],
unique: false,
null_searchable: true,
contested_index: None,
},
];
Expand All @@ -366,6 +387,7 @@ mod tests {
ascending: false,
}],
unique: false,
null_searchable: true,
contested_index: None,
}];

Expand Down Expand Up @@ -399,6 +421,7 @@ mod tests {
ascending: false,
}],
unique: false,
null_searchable: true,
contested_index: None,
}];

Expand All @@ -415,6 +438,7 @@ mod tests {
},
],
unique: false,
null_searchable: true,
contested_index: None,
}];

Expand Down Expand Up @@ -454,6 +478,7 @@ mod tests {
},
],
unique: false,
null_searchable: true,
contested_index: None,
}];

Expand All @@ -464,6 +489,7 @@ mod tests {
ascending: false,
}],
unique: false,
null_searchable: true,
contested_index: None,
}];

Expand Down
2 changes: 2 additions & 0 deletions packages/rs-dpp/src/data_contract/document_type/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ pub mod methods;
pub use index::*;
mod index_level;
pub use index_level::IndexLevel;
pub use index_level::IndexLevelTypeInfo;
pub use index_level::IndexType;

#[cfg(feature = "random-documents")]
Expand Down Expand Up @@ -42,6 +43,7 @@ mod property_names {
pub const REQUIRES_IDENTITY_DECRYPTION_BOUNDED_KEY: &str =
"requiresIdentityDecryptionBoundedKey";
pub const INDICES: &str = "indices";
pub const NULL_SEARCHABLE: &str = "nullSearchable";
pub const PROPERTIES: &str = "properties";
pub const POSITION: &str = "position";
pub const REQUIRED: &str = "required";
Expand Down
7 changes: 7 additions & 0 deletions packages/rs-dpp/src/document/accessors/v0/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,13 @@ pub trait DocumentV0Setters: DocumentV0Getters {
}
}

/// Removes the value under the given path.
/// The path supports syntax from the `lodash` JS library. Example: "root.people[0].name".
/// If parents are not present, they will be automatically created.
fn remove(&mut self, path: &str) -> Option<Value> {
self.properties_mut().remove(path)
}

/// Sets a `u8` value for the specified property name.
fn set_u8(&mut self, property_name: &str, value: u8) {
self.properties_mut()
Expand Down
6 changes: 3 additions & 3 deletions packages/rs-dpp/src/document/extended_document/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,12 +452,12 @@ mod test {
println!(
"{:?}",
doc.properties()
.get_at_path("records.dashUniqueIdentityId")
.get_at_path("records.identity")
.expect("expected to get value")
);
assert_eq!(
doc.properties()
.get_at_path("records.dashUniqueIdentityId")
.get_at_path("records.identity")
.expect("expected to get value"),
&Value::Identifier(
bs58::decode("HBNMY5QWuBVKNFLhgBTC1VmpEnscrmqKPMXpnYSHwhfn")
Expand Down Expand Up @@ -549,7 +549,7 @@ mod test {
let string = serde_json::to_string(&document)?;

assert_eq!(
"{\"version\":0,\"$type\":\"domain\",\"$dataContractId\":\"566vcJkmebVCAb2Dkj2yVMSgGFcsshupnQqtsz1RFbcy\",\"document\":{\"$version\":\"0\",\"$id\":\"4veLBZPHDkaCPF9LfZ8fX3JZiS5q5iUVGhdBbaa9ga5E\",\"$ownerId\":\"HBNMY5QWuBVKNFLhgBTC1VmpEnscrmqKPMXpnYSHwhfn\",\"$dataContractId\":\"566vcJkmebVCAb2Dkj2yVMSgGFcsshupnQqtsz1RFbcy\",\"$protocolVersion\":0,\"$type\":\"domain\",\"label\":\"user-9999\",\"normalizedLabel\":\"user-9999\",\"normalizedParentDomainName\":\"dash\",\"preorderSalt\":\"BzQi567XVqc8wYiVHS887sJtL6MDbxLHNnp+UpTFSB0=\",\"records\":{\"dashUniqueIdentityId\":\"HBNMY5QWuBVKNFLhgBTC1VmpEnscrmqKPMXpnYSHwhfn\"},\"subdomainRules\":{\"allowSubdomains\":false},\"$revision\":1,\"$createdAt\":null,\"$updatedAt\":null,\"$transferredAt\":null,\"$createdAtBlockHeight\":null,\"$updatedAtBlockHeight\":null,\"$transferredAtBlockHeight\":null,\"$createdAtCoreBlockHeight\":null,\"$updatedAtCoreBlockHeight\":null,\"$transferredAtCoreBlockHeight\":null}}",
"{\"version\":0,\"$type\":\"domain\",\"$dataContractId\":\"566vcJkmebVCAb2Dkj2yVMSgGFcsshupnQqtsz1RFbcy\",\"document\":{\"$version\":\"0\",\"$id\":\"4veLBZPHDkaCPF9LfZ8fX3JZiS5q5iUVGhdBbaa9ga5E\",\"$ownerId\":\"HBNMY5QWuBVKNFLhgBTC1VmpEnscrmqKPMXpnYSHwhfn\",\"$dataContractId\":\"566vcJkmebVCAb2Dkj2yVMSgGFcsshupnQqtsz1RFbcy\",\"$protocolVersion\":0,\"$type\":\"domain\",\"label\":\"user-9999\",\"normalizedLabel\":\"user-9999\",\"normalizedParentDomainName\":\"dash\",\"preorderSalt\":\"BzQi567XVqc8wYiVHS887sJtL6MDbxLHNnp+UpTFSB0=\",\"records\":{\"identity\":\"HBNMY5QWuBVKNFLhgBTC1VmpEnscrmqKPMXpnYSHwhfn\"},\"subdomainRules\":{\"allowSubdomains\":false},\"$revision\":1,\"$createdAt\":null,\"$updatedAt\":null,\"$transferredAt\":null,\"$createdAtBlockHeight\":null,\"$updatedAtBlockHeight\":null,\"$transferredAtBlockHeight\":null,\"$createdAtCoreBlockHeight\":null,\"$updatedAtCoreBlockHeight\":null,\"$transferredAtCoreBlockHeight\":null}}",
string
);

Expand Down
Loading
Loading