Skip to content

Conversation

@iorveth
Copy link
Contributor

@iorveth iorveth commented Sep 9, 2020

The scope of this PR includes:

  1. content_directory module runtime integration
  2. content_directory and working_group & membership modules integration related work, implemented through content_directory::ActorAuthenticator trait.

Closes #1281, #1275

@iorveth iorveth self-assigned this Sep 9, 2020
@iorveth iorveth changed the base branch from content_directory_second_try to content_dir_2 September 10, 2020 07:03
@iorveth iorveth changed the base branch from content_dir_2 to content_directory_second_try September 10, 2020 07:03
@iorveth iorveth marked this pull request as ready for review September 15, 2020 10:47
pub const PropertyDescriptionLengthConstraint: InputValidationLengthConstraint = InputValidationLengthConstraint::new(1, 500);
pub const ClassNameLengthConstraint: InputValidationLengthConstraint = InputValidationLengthConstraint::new(1, 49);
pub const ClassDescriptionLengthConstraint: InputValidationLengthConstraint = InputValidationLengthConstraint::new(1, 500);

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need spaces between lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose is to make constraints of different types distinguishable. I can remove them, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please. Remove or introduce explicit comments for different types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Removed them in content directory mock.rs as well. d993a64

Copy link
Contributor

@shamil-gadelshin shamil-gadelshin left a comment

Choose a reason for hiding this comment

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

We need to implement the working_group for Instance3, check StorageWorkingGroupInstance implementation for details.

type Nonce = u64;
type ClassId = u64;
type EntityId = u64;

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need spaces between lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose is to make constraints of different types distinguishable. I can remove them, though.


impl<T: Trait> core::fmt::Debug for Actor<T> {
#[cfg(feature = "std")]
fn fmt(&self, formatter: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove conditional compilation for this and other Debug implementations>

Copy link
Contributor Author

@iorveth iorveth Sep 15, 2020

Choose a reason for hiding this comment

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

I still getting errors, related to T: Trait inheritance, when conditional compilation enabled here. I hope this will be fixed with further refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Please, keep in mind to remove those implemented traits after the refactoring.


Ok(worker)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment for a public method, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 9d6bbb0

@@ -0,0 +1,47 @@
use crate::{AccountId, MemberId, Runtime};

// The content directory working group instance alias.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is already taken. Take instance3 instead. Also to prevent future errors - please move this line to the lib.rs closer to the Instance2 declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. But i saw, both service-discovery and storage modules are using Instance2 Is that correct case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here b9c741e

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Because they use the same working group.

Copy link
Member

@bedeho bedeho left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@shamil-gadelshin shamil-gadelshin left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants