bugfix-2972 | rusk-wallet: Refactor wallet struct to make impossible states impossible #3091
Conversation
welf
added
fix:vulnerability
welf
force-pushed
the
bugfix-2972-rusk-wallet-refactor-wallet-struct-to-make-impossible-states-impossible
branch
from
bddee85 to
1bcc58b
Compare
welf
force-pushed
the
bugfix-2972-rusk-wallet-refactor-wallet-struct-to-make-impossible-states-impossible
branch
from
1bcc58b to
e7c803f
Compare
welf
force-pushed
the
bugfix-2972-rusk-wallet-refactor-wallet-struct-to-make-impossible-states-impossible
branch
3 times, most recently
from
5c4c141 to
3d6fabe
Compare
| #[derive(Debug, Clone)] | ||
| pub struct WalletFile { | ||
| path: WalletPath, | ||
| pwd: Vec<u8>, |
There was a problem hiding this comment.
Saving the password here in the memory is necessary? I don't think its secure also given this struct is gonna live for the duration of the wallet's instance. Can we have it like before where the password wasn't stored in memory or the ptr to it isn't stored in memory
There was a problem hiding this comment.
I didn't touch the WalletFile shape and it had the pwd property before refactoring.
| /// Name of the network | ||
| pub network: Option<String>, | ||
| network: Option<String>, |
There was a problem hiding this comment.
Why do we have network in the wallet path? It seems irrelevant to have it here, we have the settings.rs file in the binary folder (which should be moved to the library IMO) which has the network there, I don't think we should duplicate that
There was a problem hiding this comment.
I didn't touch the WalletPath shape and it had the network property before refactoring.
| use crate::Error; | ||
|
|
||
| /// Provides access to a secure wallet file | ||
| pub trait SecureWalletFile: Debug + Send + Sync + Clone { |
There was a problem hiding this comment.
We have 2 abstractions now, SecureWalletFile and WalletFilePath I'm not a big fan of abstractions themselves and we should only keep these abstractions if a user is going to use them as a library and something needs to be implemented. IMO we can remove both of these abstractions since we only have one implementor and keep it as simple as we can
| } | ||
|
|
||
| /// Get the seed and address from the file | ||
| fn get_seed_and_address(&self) -> Result<(Seed, u8), Error> { |
There was a problem hiding this comment.
This logic should be inside the file.rs where the rest of DatFileVersion structs and magic numbers are defined
|
|
||
| /// Sets the network name for different cache locations. | ||
| /// e.g, devnet, testnet, etc. | ||
| fn set_network_name(&mut self, network: Option<String>) { |
There was a problem hiding this comment.
We're duplicating a lot of methods here, we have 2 traits that have duplicate methods why is that?
There was a problem hiding this comment.
We are dealing primarily with Wallet that has the file: F property. The user of the wallet doesn't have to know how the file: F is implemented and that it has the associated PathBufferWrapper. Hence I re-exported getters of the PathBufferWrapper in the SecureWalletFile trait to make the file's API more user-friendly and enable user to call all necessary functions from the file.
welf
force-pushed
the
bugfix-2972-rusk-wallet-refactor-wallet-struct-to-make-impossible-states-impossible
branch
from
3d6fabe to
bc482df
Compare
welf
force-pushed
the
bugfix-2972-rusk-wallet-refactor-wallet-struct-to-make-impossible-states-impossible
branch
from
bc482df to
e634e9f
Compare
Closes #2972
versionto thefileproperty inWallet. If we have a valid wallet file, then we know its version.WalletPathcreation methods.WalletPathstruct creation (WalletPath:new,WalletPath::try_from, andWalletPath::from_str).WalletFilePathandSecureWalletFiletraits