-
Notifications
You must be signed in to change notification settings - Fork 2
Introduce Signer interface for key management (breaking changes, moving to SDK 2.0) #21
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
Conversation
91fdb64 to
ffc1246
Compare
eed874e to
0a823c1
Compare
0a823c1 to
d0fc0e0
Compare
|
My only qualm is around the |
|
I agree with Shay, |
|
The file |
|
I don't understand the why you erased the contracts clone, and used a local versions of it (input.json and output.json). Without the clone you might miss changes |
| expect(accountIdLength).toEqual(32); | ||
| }); | ||
|
|
||
| it("should create Orbs.Client instance", async () => { |
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.
These lines were just moved, right?
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.
Yeah. I just moved to async/await syntax.
|
I have removed the contracts clone because it's not really compatible with the old format anymore (because keys moved to a different place in the config as they are not parameters of the calls anymore). We won't miss the changes because we run this e2e against gamma, so there is no difference if the input jsons come from outside or are stored here and can be refactored. |
|
I believe I have resolved all the issues. |
|
Great job!! |
Before:
After:
Notice that references to
publicKeyandprivateKeywere removed and instead you have to passnew DefaultSigner({publicKey, privateKey})which encapsulates signing.The purpose of this change is to be able to inject the signer from the outside without exposing the keys, a step forward to implementing a wallet like Metamask.