-
Notifications
You must be signed in to change notification settings - Fork 440
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
Fix Serialisation of BIP32 Keystore #278
Conversation
About the compatibility I will bump a major version. Thank you for mentioning this. |
@podkovyrin is there any idea, how we can make internal migration from the old address-path storage version? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's how it should looks like. Just want to porpose to make smooth experience for existing users and add migration function at init + deprecated flag for old parameter.
I apreciate quality and amount of your work. And if you will done thins migrations I would be glad in increase at least 2 times this bounty. 👍
|
||
import Foundation | ||
|
||
public struct PathAddressStorage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This struct works much better. great design 👍
@@ -44,8 +41,10 @@ public class BIP32Keystore: AbstractKeystore { | |||
// -------------- | |||
|
|||
public var keystoreParams: KeystoreParamsBIP32? | |||
public var paths: [String:EthereumAddress] = [String:EthereumAddress]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea to set this variable as deprecated:
@available(*, deprecated, message: "Please use addressStorage instead")
Another important thing worth mentioning that v3 is compatible with Eth Keystore Format used across multiple wallets. In my case, I didn't care about that compatibility but for some, it might be a serious issue. As a workaround, an importer/exporter can be introduced which would solve the migration problem at the same time. Also, it's probably a good idea to name this version like |
@podkovyrin well, your’re right, it's impossible to predict order from old version. But it would be possible and good practive to migrate them in any order, so developers can update the version without refactoring (and writing migration in their own code). |
so, there are 2 options on your choose @podkovyrin:
|
Whatever :) This migration is quite time-consuming and I would make it in a week or two. |
Agree. Well, I like your style and agree that it takes time. |
Be aware that this change breaks backward compatibility.
fixes #257