Implementing getBalances, getBalance, getAddress and canSign methods #24
Conversation
erdimaden
force-pushed
the
feat/balance-and-address-changes
branch
2 times, most recently
from
aabf433 to
41a52d9
Compare
erdimaden
force-pushed
the
feat/balance-and-address-changes
branch
from
41a52d9 to
c04e25e
Compare
erdimaden
changed the title
| Coinbase.apiClients.address!.createAddress = mockReturnValue(mockAddressModel); | ||
| wallet = await Wallet.create(); | ||
| }); | ||
| it("should return true when the wallet initialized ", () => { |
There was a problem hiding this comment.
Can we test when the wallet does not have a seed?
There was a problem hiding this comment.
@alex-stone Based on my understanding, wallet = await Wallet.create(); does not provide an option to include or exclude a seed. Regardless of whether we use Wallet.create or Wallet.init, the Wallet class will always have a master wallet after creation. I added this method to ensure parity with the Ruby SDK. Please let me know if there's any specific aspect you're concerned about or if I have misunderstood your point.
There was a problem hiding this comment.
Synced offline: Let's add that when we add listing wallets support
src/coinbase/tests/wallet_test.ts
Outdated
| @@ -171,4 +184,156 @@ describe("Wallet Class", () => { | |||
| expect(newWallet).toBeInstanceOf(Wallet); | |||
| }); | |||
| }); | |||
|
|
|||
| describe(".defaultAddress", () => { | |||
There was a problem hiding this comment.
If we're aiming for consistency with ruby and are using the . prefixes, we should do:
describe(".classMethod", ...
and
describe("#instanceMethod", ...
E.g.
| describe(".defaultAddress", () => { | |
| describe("#defaultAddress", () => { |
There was a problem hiding this comment.
Sure, I'm going to update this. I will also update all test files. (tracking with PSDK-165)
erdimaden
changed the title
| * @returns Whether the Wallet has a seed with which to derive keys and sign transactions. | ||
| */ | ||
| public canSign(): boolean { | ||
| return this.master.publicKey !== undefined; |
There was a problem hiding this comment.
Shouldn't this just be this.publicKey !== undefined ?
There was a problem hiding this comment.
In the Ruby SDK, we check with the master wallet. Additionally, we do not store the public key directly in the Wallet class. Instead, the privateKey or publicKey should be accessible through the master wallet.
Ruby implementation:
| const defaultAddress = wallet.defaultAddress(); | ||
| const address = wallet.getAddress(defaultAddress?.getId()); | ||
| expect(address).toBeInstanceOf(Address); | ||
| expect(address?.getId()).toBe(address.getId()); |
There was a problem hiding this comment.
Could also have this just assert on the mockAddressModel.AddresId to reduce coupling on getAddress behavior.
| }); | ||
| }); | ||
|
|
||
| describe(".networkId", () => { |
There was a problem hiding this comment.
This and wlaletId should probably be # too
There was a problem hiding this comment.
Sure, I will have a PR for this changes (PSDK-165)
| Coinbase.apiClients.address!.createAddress = mockReturnValue(mockAddressModel); | ||
| wallet = await Wallet.create(); | ||
| }); | ||
| it("should return true when the wallet initialized ", () => { |
There was a problem hiding this comment.
Synced offline: Let's add that when we add listing wallets support
What changed? Why?
I am adding the
getBalances,getBalance,getAddress, andcanSignmethods, along with their respective test cases.Qualified Impact