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.

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

Generalize KeyFile to EncryptedFile #14

Merged
merged 8 commits into from
Feb 22, 2024
Merged

Generalize KeyFile to EncryptedFile #14

merged 8 commits into from
Feb 22, 2024

Conversation

KingGorrin
Copy link
Collaborator

@KingGorrin KingGorrin commented Jan 18, 2024

This PR contains a generalization of the KeyFile and InvalidKeyStorePath class.

Generalize KeyFile to EncryptedFile

The KeyFile can be used multifunctionally if the KeyStore part is removed from the KeyFile class. The key file then changes into a non-specific encrypted file.

The calling party then becomes responsible for encoding and decoding the encrypted data, rather than the key file itself.

The key file contained a baseAddress field that was not used. Instead of resolving the baseAddress directly within the implementation, It now accepts a metadata argument, containing key/value pairs. This keeps the class generalized and allows the addition of a walletType property to solve the issue of differiating between different wallet types.

Encrypted files for hardware wallets store different data than keystore wallets. The keystore manager will validate the correct wallet type only if the field is present in the metadata; otherwise it assumes its a KeyStore.

Other changes

  • Remove unused import
  • Rename InvalidKeyStorePath to WalletException
  • Make KeyStoreManager stateless
  • Add unit test for EncryptedFile
  • Update example

@KingGorrin KingGorrin self-assigned this Jan 18, 2024
@KingGorrin KingGorrin added the enhancement New feature or request label Jan 18, 2024
@KingGorrin
Copy link
Collaborator Author

@alienc0der objected to the removal of the baseAddress.

The baseAddress from the keystore should be present even though it's not used as a means of identifying a particular keystore/key file. For example, Ethereum puts the address in the file name prepended by an UTC timestamp

Another reason to keep the baseAddress is to keep the implementation backwards compatible.

Instead of supplying the baseAddress directly to the EncryptedFile, I propose to let it accept metadata as set of key/value properties. This keeps the class generalized and allows the addition of a walletType property to solve the issue of differiating between different wallet types.

For example: encrypted files for hardware wallets store different data than keystore wallets. Currently Syrius maintains the wallet type in the hive db, just like how it's done with the wallet path. The wallet type determines how Syrius should parse the contents of the encrypted file. It is better to store this information inside the encrypted file.

Being able to supply the encrypted file with extra metadata like baseAddress and walletType solves both problems without making the implementation wallet specific.

@alienc0der alienc0der merged commit e888691 into master Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants