-
Notifications
You must be signed in to change notification settings - Fork 4
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.
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
Review js-id usage for NodeId, VaultId, NotificationId, PermId, ClaimId, Gestalts and GenericIdTypes.ts
#299
Comments
I want to know if we actually need the Furthermore in other places we see literal strings like this:
I'm not sure what was the reason for doing this. This seems overly complicated:
Which is currently in the Vault Id should be using Anyway some of the function names I need to review and see if it's appropriate. Other places where the same thing is happening:
|
GenericIdTypes.ts
GenericIdTypes.ts
Made this a high level issue overall to examine all Id usage. |
Any usage of ES6 Maps does not require the conversion of @tegefaulkes can you make sure that in your nodes refactoring, that if Note that if it is being put into a POJO, there are special provisions where |
The The vault map (ES6 Map) can just use |
In function isVaultId(arg: any) {
return isId<VaultId>(arg);
}
/**
* This will return arg as a valid VaultId or throw an error if it can't be converted.
* This will take a multibase string of the ID or the raw Buffer of the ID.
* @param arg - The variable we wish to convert
* @throws vaultsErrors.ErrorInvalidVaultId if the arg can't be converted into a VaultId
* @returns VaultId
*/
function makeVaultId(arg: any): VaultId {
return makeId<VaultId>(arg);
}
function isVaultIdPretty(arg: any): arg is VaultIdPretty {
return isIdString<VaultIdPretty>(arg);
}
function makeVaultIdPretty(arg: any): VaultIdPretty {
return makeIdString<VaultIdPretty>(arg);
} Which are just thin wrappers around functions inside the So I think some less magic is appropriate here. For encoded vault ids, it should be sufficient to just have 2 functions:
This is because we will always be using the multi-base encoding for any kind of string form. No need to also make ids from buffers. |
They're typeGuards for easy conversion between types. the generic types were for the basic conversions while the vault ones were for specifically the vaultIds. this was to keep seperation between NodeIds and VaultIds as different opaque types while they were technically the same ID type underneath. But yeah, the type functions could be tidied up. |
This reduces the list of vault ID utility functions to just: const vaultIdGenerator = new IdRandom();
function generateVaultId(): VaultId {
return vaultIdGenerator.get();
}
function encodeVaultId(vaultId: VaultId): string {
return idUtils.toMultibase(vaultId, 'base58btc');
}
function decodeVaultId(vaultIdString: string): VaultId | undefined {
return idUtils.fromMultibase(vaultIdString);
} And that's all that is needed for vaults domain. |
Yes that's right, but I think simpler solutions can suffice. For example instead of returning The generic Id utilities adds bit too much magic for my liking atm. I also prefer most domain exceptions to be in the procedural areas of the code instead of the utility functions. Utility functions should be as simple as possible. |
Currently However I want to change this up a bit. I want This means where I want a string form of So right now the encoded string form of |
The |
So it seems that what we should do is:
Here's an example of extending the type NodeId = NodeIdInternal & number;
class NodeIdInternal extends IdInternal {
public encode() {
return utils.toMultibase(this, 'base32hex');
}
}
// The `create` static method always returns `Id`, it's compatible with `NodeId` type here
const nodeId = NodeIdInternal.create() as NodeId;
let x: NodeId = nodeId;
// this is not a type error
x.encode(); |
@tegefaulkes can you link the new PRs to this issue as well. |
Specification
The current
GenericIdTypes
module is slightly out of place within the larger architecture of thejs-polykey
codebase. We should find some time to review this, and refactor its required implementation to more suitable areas of the codebase. It was originally introduced in order to facilitate usage of thejs-id
module within Polykey.Additional context
js-id
too) - discussion originally from here https://vimeo.com/manage/videos/652339635 5:00 - 17:30cloneVault
#253 - vault id usage in EFS and when cloningTasks
IdInternal
class, this requires a new PR injs-id
. IncludetoMultibase
andtoUUID
andtoBuffer
as instance methods, and thefromString
,fromMultibase
,fromUUID
, andfromBuffer
as static methods (these represent alternatives of creating theIdInternal
). Make sure that these functions returnId
as the type and notIdInternal
.NodeId
as a proxy/singleton instance #254 and relates to Review js-id usage for NodeId, VaultId, NotificationId, PermId, ClaimId, Gestalts andGenericIdTypes.ts
#299 (this requires more work to be done, not just node id).NodeId
which is an opaque alias ofId
(which itself is an alias ofIdInternal & number
).NodeId
is expecting an encoded string. These will need to be changed to expect a sort oftype NodeIdEncoded = string;
, you still need anodesUtils.encodeNodeId
.NodeId
should be insidenodes/utils.ts
. This should be similar to how I'm doing it forVaultId
. An alternative might be to create class extension or generics, but this seems complicated.NodeId
will mostly occur inside the nodes domain, in particular theNodeGraph
can use theUint8Array
directly. Note that thejs-db
currently doesn't yet allow direct usage of theId
, because it requires aBuffer
type. However there is an issue UsebufferWrap
to supportArrayBuffer
,Uint8Array
andBuffer
js-db#3 to allow it to eventually takeId
, so for now, just useidUtils.toBuffer
andidUtils.fromBuffer
when interacting with the DB.NodeManager
toNodeConnectionManager
#310 on top to benefit from it.GenericIdTypes.ts
can be removed once all other areas of code stop using it.The text was updated successfully, but these errors were encountered: