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 10 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
52 changes: 4 additions & 48 deletions packages/dpns-contract/test/unit/dpnsContract.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ describe('DPNS Contract', () => {
normalizedParentDomainName: 'dash',
preorderSalt: crypto.randomBytes(32),
records: {
dashUniqueIdentityId: await generateRandomIdentifier(),
identity: await generateRandomIdentifier(),
},
subdomainRules: {
allowSubdomains: false,
Expand Down Expand Up @@ -468,54 +468,10 @@ describe('DPNS Contract', () => {
});

describe('Dash Identity', () => {
it('should have either `dashUniqueIdentityId` or `dashAliasIdentityId`', async () => {
rawDomainDocument.records = {
dashUniqueIdentityId: identityId,
dashAliasIdentityId: identityId,
};

const document = dpp.document.create(dataContract, identityId, 'domain', rawDomainDocument);
const validationResult = document.validate(dpp.protocolVersion);
const error = expectJsonSchemaError(validationResult);

expect(error.keyword)
.to
.equal('maxProperties');
expect(error.instancePath)
.to
.equal('/records');
});

describe('dashUniqueIdentityId', () => {
it('should no less than 32 bytes', async () => {
rawDomainDocument.records = {
dashUniqueIdentityId: crypto.randomBytes(30),
};

expect(() => {
dpp.document.create(dataContract, identityId, 'domain', rawDomainDocument);
})
.to
.throw();
});

it('should no more than 32 bytes', async () => {
rawDomainDocument.records = {
dashUniqueIdentityId: crypto.randomBytes(64),
};

expect(() => {
dpp.document.create(dataContract, identityId, 'domain', rawDomainDocument);
})
.to
.throw();
});
});

describe('dashAliasIdentityId', () => {
describe('identity record', () => {
it('should no less than 32 bytes', async () => {
rawDomainDocument.records = {
dashAliasIdentityId: crypto.randomBytes(30),
identity: crypto.randomBytes(30),
};

expect(() => {
Expand All @@ -527,7 +483,7 @@ describe('DPNS Contract', () => {

it('should no more than 32 bytes', async () => {
rawDomainDocument.records = {
dashAliasIdentityId: crypto.randomBytes(64),
identity: crypto.randomBytes(64),
};

expect(() => {
Expand Down
18 changes: 9 additions & 9 deletions packages/platform-test-suite/test/e2e/dpns.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ describe('DPNS', () => {
// skip if DPNS owner private key is not passed and use `dash` in tests above
it('should be able to register a TLD', async () => {
createdTLD = await ownerClient.platform.names.register(newTopLevelDomain, {
dashAliasIdentityId: identity.getId(),
identity: identity.getId(),
}, identity);

// Additional wait time to mitigate testnet latency
Expand Down Expand Up @@ -148,7 +148,7 @@ describe('DPNS', () => {

try {
await client.platform.names.register(getRandomDomain(), {
dashAliasIdentityId: identity.getId(),
identity: identity.getId(),
}, identity);
} catch (e) {
broadcastError = e;
Expand All @@ -161,7 +161,7 @@ describe('DPNS', () => {

it('should be able to register a second level domain', async () => {
registeredDomain = await client.platform.names.register(`${secondLevelDomain}0.${topLevelDomain}`, {
dashUniqueIdentityId: identity.getId(),
identity: identity.getId(),
}, identity);

// Additional wait time to mitigate testnet latency
Expand All @@ -179,7 +179,7 @@ describe('DPNS', () => {
const domain = `${secondLevelDomain}O.${topLevelDomain}`;

await client.platform.names.register(domain, {
dashAliasIdentityId: identity.getId(),
identity: identity.getId(),
}, identity);

expect.fail('should throw error');
Expand All @@ -199,7 +199,7 @@ describe('DPNS', () => {
const domain = `${getRandomDomain()}.${getRandomDomain()}`;

await client.platform.names.register(domain, {
dashAliasIdentityId: identity.getId(),
identity: identity.getId(),
}, identity);

expect.fail('should throw error');
Expand Down Expand Up @@ -252,8 +252,8 @@ describe('DPNS', () => {

it('should be able to resolve domain by it\'s record', async () => {
const [document] = await client.platform.names.resolveByRecord(
'dashUniqueIdentityId',
registeredDomain.getData().records.dashUniqueIdentityId,
'identity',
registeredDomain.getData().records.identity,
);

const rawDocument = document.toObject();
Expand Down Expand Up @@ -310,9 +310,9 @@ describe('DPNS', () => {
expect(broadcastError.code).to.equal(40500);
});

it('should not be able to register two domains with same `dashAliasIdentityId` record');
it('should not be able to register two domains with same `identity` record');

it('should be able to register many domains with same `dashAliasIdentityId` record');
it('should be able to register many domains with same `identity` record');

it('should not be able to update preorder');

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
Loading
Loading