[#1511] Refactor account representation from int to a dedicated account structure #1515
+435
−376
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… to a dedicated Account structure - First iteration of all Int account references to an instance of an Account
- The ID of account is now public co clients can read-only it
- Zip32 accounts represented with Int32 are now refactored to Zip32Account - from_account_id/fto_account_id are now refactored to AccountId
pacu
reviewed
Nov 22, 2024
| // | ||
|
|
||
| /// A [ZIP 32](https://zips.z.cash/zip-0032) account index. | ||
| public struct Zip32Account: Equatable, Codable, Hashable { |
There was a problem hiding this comment.
This is a much needed change!
pacu
suggested changes
Nov 22, 2024
|
|
||
| /// A [ZIP 32](https://zips.z.cash/zip-0032) account index. | ||
| public struct Zip32Account: Equatable, Codable, Hashable { | ||
| public let index: Int32 |
There was a problem hiding this comment.
I would take the opportunity to make this UInt32 and internally then convert to Int32.
Also I the derivation tool in the FFI derives the Network Id as UInt32 when u32 is specified in Rust, so we could make that change later.
If the public API uses UInt32 then this change in Rust won't imply a deprecation in the SDK's public API. then the init can get rid of the fatalError() call.
There was a problem hiding this comment.
The valid range is 0..<(1<<31) (because the high bit is used in ZIP 32 / BIP 32 to indicate "hardened" which doesn't apply to account indices), so it doesn't really matter from that point of view whether it is stored as Int32 or UInt32. That is, the fatalError from init would be needed regardless.
I've gone back and forth on this, but after careful consideration I agree that the public API should use UInt32. There are not actually many places where we have to convert when calling zcashlc_* functions that take an account parameter, and this PR is changing all of them anyway.
Suggested change
| public let index: Int32 | |
| public let index: UInt32 |
daira
reviewed
Nov 23, 2024
| @@ -103,11 +103,13 @@ class SendViewController: UIViewController { | |||
| } | |||
|
|
|||
| func updateBalance() async { | |||
| let account = Zip32Account(0) | |||
There was a problem hiding this comment.
Even though this is only sample code, I'd suggest making this an attribute of SendViewController so that it's only hardcoded once. (There's no need to do anything more than that.)
daira
reviewed
Nov 23, 2024
| } | ||
| } | ||
|
|
||
| public struct AccountId: Equatable, Codable, Hashable { |
There was a problem hiding this comment.
I would prefer this to be called LocalAccountId. (See also zcash/librustzcash#1631 (comment) .)
Suggested change
| public struct AccountId: Equatable, Codable, Hashable { | |
| public struct LocalAccountId: Equatable, Codable, Hashable { |
There was a problem hiding this comment.
No, this will become the account ID containing the UUID; the local one will not be visible to the Swift code.
daira
reviewed
Nov 23, 2024
| } | ||
|
|
||
| public struct AccountId: Equatable, Codable, Hashable { | ||
| public let index: Int |
There was a problem hiding this comment.
Suggested change
| public let index: Int | |
| public let id: Int |
daira
reviewed
Nov 23, 2024
Comment on lines
+24
to
+30
| public init(_ index: Int) { | ||
| guard index >= 0 else { | ||
| fatalError("Account index must be >= 0. Input value is \(index).") | ||
| } | ||
|
|
||
| self.index = index | ||
| } |
There was a problem hiding this comment.
Suggested change
| public init(_ index: Int) { | |
| guard index >= 0 else { | |
| fatalError("Account index must be >= 0. Input value is \(index).") | |
| } | |
| self.index = index | |
| } | |
| /// - Parameter id: the local account id, which must be nonnegative. | |
| public init(_ id: Int) { | |
| guard id >= 0 else { | |
| fatalError("Local account id must be >= 0. Input value is \(id).") | |
| } | |
| self.id = id | |
| } |
daira
reviewed
Nov 23, 2024
| @@ -22,7 +22,8 @@ extension SaplingParamsAction: Action { | |||
|
|
|||
| func run(with context: ActionContext, didUpdate: @escaping (CompactBlockProcessor.Event) async -> Void) async throws -> ActionContext { | |||
| logger.debug("Fetching sapling parameters") | |||
| try await saplingParametersHandler.handleIfNeeded() | |||
| // TODO: This is hardcoded Zip32Account for index 0, must be updated | |||
There was a problem hiding this comment.
Please file an issue like the lint says.
There was a problem hiding this comment.
We already have one: #1512
daira
reviewed
Nov 23, 2024
| @@ -11,9 +11,9 @@ protocol SentNoteEntity { | |||
| var id: Int { get } | |||
| var transactionId: Int { get } | |||
| var outputIndex: Int { get } | |||
| var fromAccount: Int { get } | |||
| var fromAccount: AccountId { get } | |||
There was a problem hiding this comment.
Suggested change
| var fromAccount: AccountId { get } | |
| var fromAccount: LocalAccountId { get } |
and every similar case.
There was a problem hiding this comment.
No, per my above comment (it would just get renamed back in the next PR).
daira
reviewed
Nov 23, 2024
Comment on lines
+46
to
48
| /// - Parameter accountIndex:account that the key can spend from | ||
| /// - Throws: `rustDeriveUnifiedSpendingKey` if rust layer returns error. | ||
| func deriveUnifiedSpendingKey(from seed: [UInt8], accountIndex: Int32) throws -> UnifiedSpendingKey |
There was a problem hiding this comment.
Suggested change
| /// - Parameter accountIndex:account that the key can spend from | |
| /// - Throws: `rustDeriveUnifiedSpendingKey` if rust layer returns error. | |
| func deriveUnifiedSpendingKey(from seed: [UInt8], accountIndex: Int32) throws -> UnifiedSpendingKey | |
| /// - Parameter accountIndex: the ZIP 32 index of the account from which the key can spend | |
| /// - Throws: `rustDeriveUnifiedSpendingKey` if rust layer returns error. | |
| func deriveUnifiedSpendingKey(from seed: [UInt8], accountIndex: Zip32AccountIndex) throws -> UnifiedSpendingKey |
daira
reviewed
Nov 23, 2024
| /// - Throws: | ||
| /// - `derivationToolInvalidAccount` if the `accountIndex` is invalid. | ||
| /// - `derivationToolInvalidAccount` if the `account.index` is invalid. |
There was a problem hiding this comment.
I think this error cannot occur here; please check.
Suggested change
| /// - `derivationToolInvalidAccount` if the `account.index` is invalid. |
daira
reviewed
Nov 23, 2024
| @@ -94,9 +93,9 @@ protocol ZcashKeyDerivationBackendWelding { | |||
| /// | |||
| /// - Parameter contextString: a globally-unique non-empty sequence of at most 252 bytes that identifies the desired context. | |||
| /// - Parameter seed: `[Uint8]` seed bytes | |||
| /// - Parameter accountNumber: `Int32` with the account number | |||
| /// - Parameter accountIndex: `Int32` with the account number | |||
There was a problem hiding this comment.
Suggested change
| /// - Parameter accountIndex: `Int32` with the account number | |
| /// - Parameter accountIndex: the ZIP 32 index of the account |
daira
reviewed
Nov 23, 2024
| /// - Throws: | ||
| /// - `derivationToolInvalidAccount` if the `accountIndex` is invalid. | ||
| /// - `derivationToolInvalidAccount` if the `account.index` is invalid. |
There was a problem hiding this comment.
Change accountIndex: Int32 to accountIndex: Zip32AccountIndex below. Then, this exception cannot occur because the account index will have been checked to be in the valid range (which is all that is required here; it does not have to be an index of an account known to the wallet).
Suggested change
| /// - `derivationToolInvalidAccount` if the `account.index` is invalid. |
daira
reviewed
Nov 23, 2024
Comment on lines
+12
to
+15
| public init(_ index: Int32) { | ||
| guard index >= 0 else { | ||
| fatalError("Account index must be >= 0. Input value is \(index).") | ||
| } |
There was a problem hiding this comment.
Suggested change
| public init(_ index: Int32) { | |
| guard index >= 0 else { | |
| fatalError("Account index must be >= 0. Input value is \(index).") | |
| } | |
| /// - Parameter index: the ZIP 32 account index, which must be less than ``1<<31``. | |
| public init(_ index: UInt32) { | |
| guard index < (1<<31) else { | |
| fatalError("Account index must be less than 1<<31. Input value is \(index).") | |
| } |
Note that many of my other suggestions depend on having made this change.
daira
reviewed
Nov 23, 2024
| let ufvk: String | ||
| } | ||
|
|
||
| extension DbAccount: Hashable { | ||
| func hash(into hasher: inout Hasher) { | ||
| hasher.combine(account) | ||
| hasher.combine(account.index) |
There was a problem hiding this comment.
Changing the type of index to UInt32 causes no problem here.
daira
reviewed
Nov 23, 2024
|
|
||
| for i in (0 ..< Int(accountsPtr.pointee.len)) { | ||
| let account = accountsPtr.pointee.ptr.advanced(by: i).pointee | ||
| accounts.append(Int32(account.account_index)) | ||
| accounts.append(Zip32Account(Int32(account.account_index))) |
There was a problem hiding this comment.
Suggested change
| accounts.append(Zip32Account(Int32(account.account_index))) | |
| accounts.append(Zip32AccountIndex(account.account_index)) |
daira
reviewed
Nov 23, 2024
| to address: String, | ||
| value: Int64, | ||
| memo: MemoBytes? | ||
| ) async throws -> FfiProposal { | ||
| let proposal = zcashlc_propose_transfer( | ||
| dbData.0, | ||
| dbData.1, | ||
| account, | ||
| account.index, |
There was a problem hiding this comment.
Define typealias LCZip32Index = Int32 (local to this file), so that if we decide to change the type of account indices passed to zcashlc_* functions, that can be done in one place (or at least we've marked all places that need to change).
Suggested change
| account.index, | |
| LCZip32Index(account.index), |
daira
reviewed
Nov 23, 2024
| ) async throws -> FfiProposal { | ||
| let proposal = zcashlc_propose_transfer_from_uri( | ||
| dbData.0, | ||
| dbData.1, | ||
| account, | ||
| account.index, |
There was a problem hiding this comment.
Suggested change
| account.index, | |
| LCZip32Index(account.index), |
daira
reviewed
Nov 23, 2024
| let addressCStr = zcashlc_get_current_address( | ||
| dbData.0, | ||
| dbData.1, | ||
| account, | ||
| account.index, |
There was a problem hiding this comment.
Suggested change
| account.index, | |
| LCZip32Index(account.index), |
daira
reviewed
Nov 23, 2024
| let addressCStr = zcashlc_get_next_available_address( | ||
| dbData.0, | ||
| dbData.1, | ||
| account, | ||
| account.index, |
There was a problem hiding this comment.
Suggested change
| account.index, | |
| LCZip32Index(account.index), |
daira
reviewed
Nov 23, 2024
| } | ||
|
|
||
| let balance = zcashlc_get_total_transparent_balance_for_account( | ||
| dbData.0, | ||
| dbData.1, | ||
| networkType.networkId, | ||
| account | ||
| account.index |
There was a problem hiding this comment.
Suggested change
| account.index | |
| LCZip32Index(account.index) |
daira
reviewed
Nov 23, 2024
| } | ||
|
|
||
| let balance = zcashlc_get_verified_transparent_balance_for_account( | ||
| dbData.0, | ||
| dbData.1, | ||
| networkType.networkId, | ||
| account, | ||
| account.index, |
There was a problem hiding this comment.
Suggested change
| account.index, | |
| LCZip32Index(account.index), |
daira
reviewed
Nov 23, 2024
| let encodedKeysPtr = zcashlc_list_transparent_receivers( | ||
| dbData.0, | ||
| dbData.1, | ||
| account, | ||
| account.index, |
There was a problem hiding this comment.
Suggested change
| account.index, | |
| LCZip32Index(account.index), |
daira
reviewed
Nov 23, 2024
| memo: MemoBytes?, | ||
| shieldingThreshold: Zatoshi, | ||
| transparentReceiver: String? | ||
| ) async throws -> FfiProposal? { | ||
| let proposal = zcashlc_propose_shielding( | ||
| dbData.0, | ||
| dbData.1, | ||
| account, | ||
| account.index, |
There was a problem hiding this comment.
Suggested change
| account.index, | |
| LCZip32Index(account.index), |
daira
reviewed
Nov 23, 2024
| @@ -228,15 +228,15 @@ public protocol Synchronizer: AnyObject { | |||
| memo: Memo? | |||
| ) async throws -> ZcashTransaction.Overview | |||
|
|
|||
| /// Attempts to propose fulfilling a [ZIP-321](https://zips.z.cash/zip-0321) payment URI using the given `accountIndex` | |||
| /// Attempts to propose fulfilling a [ZIP-321](https://zips.z.cash/zip-0321) payment URI using the given `account.index` | |||
There was a problem hiding this comment.
Suggested change
| /// Attempts to propose fulfilling a [ZIP-321](https://zips.z.cash/zip-0321) payment URI using the given `account.index` | |
| /// Attempts to propose fulfilling a [ZIP-321](https://zips.z.cash/zip-0321) payment URI by spending from the ZIP 32 account with the given index. |
daira
reviewed
Nov 23, 2024
|
|
||
| /// Creates a proposal for transferring funds to the given recipient. | ||
| /// | ||
| /// - Parameter accountIndex: the account from which to transfer funds. | ||
| /// - Parameter account: the account from which to transfer funds. |
There was a problem hiding this comment.
Suggested change
| /// - Parameter account: the account from which to transfer funds. | |
| /// - Parameter accountIndex: the ZIP 32 index of the account from which to transfer funds. |
and the same wording for every similar case.
daira
reviewed
Nov 23, 2024
| /// - Parameter recipient: the recipient's address. | ||
| /// - Parameter amount: the amount to send in Zatoshi. | ||
| /// - Parameter memo: an optional memo to include as part of the proposal's transactions. Use `nil` when sending to transparent receivers otherwise the function will throw an error. | ||
| /// | ||
| /// If `prepare()` hasn't already been called since creation of the synchronizer instance or since the last wipe then this method throws | ||
| /// `SynchronizerErrors.notPrepared`. | ||
| func proposeTransfer( | ||
| accountIndex: Int, | ||
| account: Zip32Account, |
There was a problem hiding this comment.
Suggested change
| account: Zip32Account, | |
| accountIndex: Zip32AccountIndex, |
and every similar case.
daira
reviewed
Nov 23, 2024
|
|
||
| /// Gets the unified address for the given account. | ||
| /// - Parameter accountIndex: the optional accountId whose address is of interest. By default, the first account is used. | ||
| /// - Parameter account: the account whose address is of interest. By default, the first account is used. |
There was a problem hiding this comment.
Suggested change
| /// - Parameter account: the account whose address is of interest. By default, the first account is used. | |
| /// - Parameter accountIndex: the ZIP 32 index of the account whose address is of interest. |
There is no default for this parameter.
daira
reviewed
Nov 23, 2024
|
|
||
| /// Gets the transparent address for the given account. | ||
| /// - Parameter accountIndex: the optional accountId whose address is of interest. By default, the first account is used. | ||
| /// - Parameter account: the account whose address is of interest. By default, the first account is used. |
There was a problem hiding this comment.
Suggested change
| /// - Parameter account: the account whose address is of interest. By default, the first account is used. | |
| /// - Parameter accountIndex: the ZIP 32 index of the account whose address is of interest. |
There is no default for this parameter.
daira
reviewed
Nov 23, 2024
|
|
||
| guard let tBalance = try await self.getAccountBalance(accountIndex: accountIndex)?.unshielded else { | ||
| guard let tBalance = try await self.getAccountBalance(account: account)?.unshielded else { |
daira
reviewed
Nov 23, 2024
| @@ -394,9 +394,9 @@ public class SDKSynchronizer: Synchronizer { | |||
| try throwIfUnprepared() | |||
|
|
|||
| // let's see if there are funds to shield | |||
| let accountIndex = Int(spendingKey.account) | |||
| let account = Zip32Account(Int32(spendingKey.account)) | |||
There was a problem hiding this comment.
Suggested change
| let account = Zip32Account(Int32(spendingKey.account)) | |
| let accountIndex = spendingKey.accountIndex |
(I am going to also suggest renaming UnifiedSpendingKey.index and changing its type to Zip32AccountIndex so that this works.)
daira
reviewed
Nov 23, 2024
| @@ -406,7 +406,7 @@ public class SDKSynchronizer: Synchronizer { | |||
| } | |||
|
|
|||
| guard let proposal = try await transactionEncoder.proposeShielding( | |||
| accountIndex: Int(spendingKey.account), | |||
| account: account, | |||
There was a problem hiding this comment.
Suggested change
| account: account, | |
| accountIndex: accountIndex, |
daira
reviewed
Nov 23, 2024
| @@ -441,7 +441,7 @@ public class SDKSynchronizer: Synchronizer { | |||
| } | |||
|
|
|||
| let proposal = try await transactionEncoder.proposeTransfer( | |||
| accountIndex: Int(spendingKey.account), | |||
| account: Zip32Account(Int32(spendingKey.account)), | |||
There was a problem hiding this comment.
Suggested change
| account: Zip32Account(Int32(spendingKey.account)), | |
| accountIndex: spendingKey.accountIndex, |
daira
reviewed
Nov 23, 2024
Comment on lines
107
to
108
| /// - Parameter numberOfAccounts: the number of accounts to use. Multiple accounts are not fully | ||
| /// supported so the default value of 1 is recommended. |
There was a problem hiding this comment.
Please check that this is correct.
Suggested change
| /// - Parameter numberOfAccounts: the number of accounts to use. Multiple accounts are not fully | |
| /// supported so the default value of 1 is recommended. | |
| /// - Parameter accountIndex: the ZIP 32 index of the account |
daira
reviewed
Nov 23, 2024
Comment on lines
+110
to
+114
| public func deriveUnifiedSpendingKey(seed: [UInt8], accountIndex: Int32) throws -> UnifiedSpendingKey { | ||
| guard accountIndex >= 0 else { | ||
| throw ZcashError.derivationToolInvalidAccount | ||
| } | ||
|
|
There was a problem hiding this comment.
Delete derivationToolInvalidAccount if there are no more occurrences.
Suggested change
| public func deriveUnifiedSpendingKey(seed: [UInt8], accountIndex: Int32) throws -> UnifiedSpendingKey { | |
| guard accountIndex >= 0 else { | |
| throw ZcashError.derivationToolInvalidAccount | |
| } | |
| public func deriveUnifiedSpendingKey(seed: [UInt8], accountIndex: Zip32AccountIndex) throws -> UnifiedSpendingKey { |
daira
reviewed
Nov 23, 2024
| /// - Throws: | ||
| /// - `derivationToolInvalidAccount` if the `accountIndex` is invalid. | ||
| /// - `derivationToolInvalidAccount` if the `account` is invalid. |
There was a problem hiding this comment.
Suggested change
| /// - `derivationToolInvalidAccount` if the `account` is invalid. |
daira
reviewed
Nov 23, 2024
Comment on lines
+149
to
+153
| public func deriveArbitraryAccountKey(contextString: [UInt8], seed: [UInt8], accountIndex: Int32) throws -> [UInt8] { | ||
| guard accountIndex >= 0 else { | ||
| throw ZcashError.derivationToolInvalidAccount | ||
| } | ||
|
|
There was a problem hiding this comment.
Delete derivationToolInvalidAccount if there are no more occurrences.
Suggested change
| public func deriveArbitraryAccountKey(contextString: [UInt8], seed: [UInt8], accountIndex: Int32) throws -> [UInt8] { | |
| guard accountIndex >= 0 else { | |
| throw ZcashError.derivationToolInvalidAccount | |
| } | |
| public func deriveArbitraryAccountKey(contextString: [UInt8], seed: [UInt8], accountIndex: Zip32AccountIndex) throws -> [UInt8] { |
daira
reviewed
Nov 23, 2024
| /// - Throws: | ||
| /// - `derivationToolInvalidAccount` if the `accountIndex` is invalid. | ||
| /// - `derivationToolInvalidAccount` if the `account` is invalid. |
There was a problem hiding this comment.
Suggested change
| /// - `derivationToolInvalidAccount` if the `account` is invalid. |
daira
reviewed
Nov 23, 2024
| @@ -140,13 +141,16 @@ public class DerivationTool: KeyDeriving { | |||
| /// | |||
| /// - Parameter contextString: a globally-unique non-empty sequence of at most 252 bytes that identifies the desired context. | |||
| /// - Parameter seed: `[Uint8]` seed bytes | |||
| /// - Parameter accountNumber: `Int` with the account number | |||
| /// - Parameter accountIndex: `Int32` with the account number | |||
There was a problem hiding this comment.
Suggested change
| /// - Parameter accountIndex: `Int32` with the account number | |
| /// - Parameter accountIndex: the ZIP 32 index of the account |
daira
reviewed
Nov 23, 2024
| @@ -113,7 +113,7 @@ class PaymentURIFulfillmentTests: ZcashTestCase { | |||
| do { | |||
| let proposal = try await coordinator.synchronizer.proposefulfillingPaymentURI( | |||
| paymentURI, | |||
| accountIndex: 0 | |||
| account: Account(0) | |||
There was a problem hiding this comment.
Suggested change
| account: Account(0) | |
| accountIndex: Zip32AccountIndex(0) |
daira
reviewed
Nov 23, 2024
| @@ -320,7 +320,7 @@ class PaymentURIFulfillmentTests: ZcashTestCase { | |||
| do { | |||
| _ = try await coordinator.synchronizer.proposefulfillingPaymentURI( | |||
| paymentURI, | |||
| accountIndex: 0 | |||
| account: Account(0) | |||
There was a problem hiding this comment.
Suggested change
| account: Account(0) | |
| accountIndex: Zip32AccountIndex(0) |
daira
reviewed
Nov 23, 2024
| @@ -86,6 +86,8 @@ class ShieldFundsTests: ZcashTestCase { | |||
| /// verify that the shielded transactions are confirmed | |||
| /// | |||
| func testShieldFunds() async throws { | |||
| let account = Account(0) | |||
There was a problem hiding this comment.
Suggested change
| let account = Account(0) | |
| let accountIndex: Zip32AccountIndex(0) |
daira
reviewed
Nov 23, 2024
| @@ -99,7 +101,7 @@ class ShieldFundsTests: ZcashTestCase { | |||
| var initialTotalBalance = Zatoshi(-1) | |||
| var initialVerifiedBalance = Zatoshi(-1) | |||
|
|
|||
| var initialTransparentBalance: Zatoshi = try await coordinator.synchronizer.getAccountBalance(accountIndex: 0)?.unshielded ?? .zero | |||
| var initialTransparentBalance: Zatoshi = try await coordinator.synchronizer.getAccountBalance(account: account)?.unshielded ?? .zero | |||
There was a problem hiding this comment.
Suggested change
| var initialTransparentBalance: Zatoshi = try await coordinator.synchronizer.getAccountBalance(account: account)?.unshielded ?? .zero | |
| var initialTransparentBalance: Zatoshi = try await coordinator.synchronizer.getAccountBalance(accountIndex: accountIndex)?.unshielded ?? .zero |
daira
reviewed
Nov 23, 2024
Comment on lines
+126
to
+127
| initialVerifiedBalance = try await synchronizer.getAccountBalance(account: account)?.saplingBalance.spendableValue ?? .zero | ||
| initialTotalBalance = try await synchronizer.getAccountBalance(account: account)?.saplingBalance.total() ?? .zero |
There was a problem hiding this comment.
Suggested change
| initialVerifiedBalance = try await synchronizer.getAccountBalance(account: account)?.saplingBalance.spendableValue ?? .zero | |
| initialTotalBalance = try await synchronizer.getAccountBalance(account: account)?.saplingBalance.total() ?? .zero | |
| initialVerifiedBalance = try await synchronizer.getAccountBalance(accountIndex: accountIndex)?.saplingBalance.spendableValue ?? .zero | |
| initialTotalBalance = try await synchronizer.getAccountBalance(accountIndex: accountIndex)?.saplingBalance.total() ?? .zero |
daira
reviewed
Nov 23, 2024
| @@ -142,7 +144,7 @@ class ShieldFundsTests: ZcashTestCase { | |||
| // at this point the balance should be all zeroes for transparent and shielded funds | |||
| XCTAssertEqual(initialTotalBalance, Zatoshi.zero) | |||
| XCTAssertEqual(initialVerifiedBalance, Zatoshi.zero) | |||
| initialTransparentBalance = (try? await coordinator.synchronizer.getAccountBalance(accountIndex: 0))?.unshielded ?? .zero | |||
| initialTransparentBalance = (try? await coordinator.synchronizer.getAccountBalance(account: account))?.unshielded ?? .zero | |||
There was a problem hiding this comment.
Suggested change
| initialTransparentBalance = (try? await coordinator.synchronizer.getAccountBalance(account: account))?.unshielded ?? .zero | |
| initialTransparentBalance = (try? await coordinator.synchronizer.getAccountBalance(accountIndex: accountIndex))?.unshielded ?? .zero |
daira
reviewed
Nov 23, 2024
| @@ -175,7 +177,7 @@ class ShieldFundsTests: ZcashTestCase { | |||
|
|
|||
| // at this point the balance should be zero for shielded, then zero verified transparent funds | |||
| // and 10000 zatoshi of total (not verified) transparent funds. | |||
| let tFundsDetectedBalance = try await coordinator.synchronizer.getAccountBalance(accountIndex: 0)?.unshielded ?? .zero | |||
| let tFundsDetectedBalance = try await coordinator.synchronizer.getAccountBalance(account: account)?.unshielded ?? .zero | |||
There was a problem hiding this comment.
Suggested change
| let tFundsDetectedBalance = try await coordinator.synchronizer.getAccountBalance(account: account)?.unshielded ?? .zero | |
| let tFundsDetectedBalance = try await coordinator.synchronizer.getAccountBalance(accountIndex: accountIndex)?.unshielded ?? .zero |
daira
reviewed
Nov 23, 2024
| @@ -204,7 +206,7 @@ class ShieldFundsTests: ZcashTestCase { | |||
| await fulfillment(of: [tFundsConfirmationSyncExpectation], timeout: 5) | |||
|
|
|||
| // the transparent funds should be 10000 zatoshis both total and verified | |||
| let confirmedTFundsBalance = try await coordinator.synchronizer.getAccountBalance(accountIndex: 0)?.unshielded ?? .zero | |||
| let confirmedTFundsBalance = try await coordinator.synchronizer.getAccountBalance(account: account)?.unshielded ?? .zero | |||
There was a problem hiding this comment.
Suggested change
| let confirmedTFundsBalance = try await coordinator.synchronizer.getAccountBalance(account: account)?.unshielded ?? .zero | |
| let confirmedTFundsBalance = try await coordinator.synchronizer.getAccountBalance(accountIndex: accountIndex)?.unshielded ?? .zero |
daira
reviewed
Nov 23, 2024
| @@ -235,7 +237,7 @@ class ShieldFundsTests: ZcashTestCase { | |||
|
|
|||
| guard shouldContinue else { return } | |||
|
|
|||
| let postShieldingBalance = try await coordinator.synchronizer.getAccountBalance(accountIndex: 0)?.unshielded ?? .zero | |||
| let postShieldingBalance = try await coordinator.synchronizer.getAccountBalance(account: account)?.unshielded ?? .zero | |||
There was a problem hiding this comment.
Suggested change
| let postShieldingBalance = try await coordinator.synchronizer.getAccountBalance(account: account)?.unshielded ?? .zero | |
| let postShieldingBalance = try await coordinator.synchronizer.getAccountBalance(accountIndex: accountIndex)?.unshielded ?? .zero |
daira
reviewed
Nov 23, 2024
| @@ -288,7 +290,7 @@ class ShieldFundsTests: ZcashTestCase { | |||
|
|
|||
| // Now it should verify that the balance has been shielded. The resulting balance should be zero | |||
| // transparent funds and `10000 - fee` total shielded funds, zero verified shielded funds. | |||
| let postShieldingShieldedBalance = try await coordinator.synchronizer.getAccountBalance(accountIndex: 0)?.unshielded ?? .zero | |||
| let postShieldingShieldedBalance = try await coordinator.synchronizer.getAccountBalance(account: account)?.unshielded ?? .zero | |||
There was a problem hiding this comment.
Suggested change
| let postShieldingShieldedBalance = try await coordinator.synchronizer.getAccountBalance(account: account)?.unshielded ?? .zero | |
| let postShieldingShieldedBalance = try await coordinator.synchronizer.getAccountBalance(accountIndex: accountIndex)?.unshielded ?? .zero |
daira
reviewed
Nov 23, 2024
| @@ -330,7 +332,7 @@ class ShieldFundsTests: ZcashTestCase { | |||
|
|
|||
| expectedBalance = try await coordinator.synchronizer.getAccountBalance()?.saplingBalance.total() ?? .zero | |||
| XCTAssertEqual(expectedBalance, Zatoshi(9000)) | |||
| let postShieldingConfirmationShieldedBalance = try await coordinator.synchronizer.getAccountBalance(accountIndex: 0)?.unshielded ?? .zero | |||
| let postShieldingConfirmationShieldedBalance = try await coordinator.synchronizer.getAccountBalance(account: account)?.unshielded ?? .zero | |||
There was a problem hiding this comment.
Suggested change
| let postShieldingConfirmationShieldedBalance = try await coordinator.synchronizer.getAccountBalance(account: account)?.unshielded ?? .zero | |
| let postShieldingConfirmationShieldedBalance = try await coordinator.synchronizer.getAccountBalance(accountIndex: accountIndex)?.unshielded ?? .zero |
daira
reviewed
Nov 23, 2024
Comment on lines
+187
to
+188
| synchronizerMock.getSaplingAddressAccountClosure = { account in | ||
| XCTAssertEqual(account, Account(3)) |
There was a problem hiding this comment.
Suggested change
| synchronizerMock.getSaplingAddressAccountClosure = { account in | |
| XCTAssertEqual(account, Account(3)) | |
| synchronizerMock.getSaplingAddressAccountIndexClosure = { accountIndex in | |
| XCTAssertEqual(accountIndex, Zip32AccountIndex(3)) |
and similarly below.
daira
reviewed
Nov 23, 2024
Comment on lines
+221
to
+224
| let testAccount = Account(3) | ||
|
|
||
| synchronizerMock.getSaplingAddressAccountClosure = { account in | ||
| XCTAssertEqual(account, testAccount) |
There was a problem hiding this comment.
Suggested change
| let testAccount = Account(3) | |
| synchronizerMock.getSaplingAddressAccountClosure = { account in | |
| XCTAssertEqual(account, testAccount) | |
| let testAccountIndex = Zip32AccountIndex(3) | |
| synchronizerMock.getSaplingAddressAccountIndexClosure = { accountIndex in | |
| XCTAssertEqual(accountIndex, testAccountIndex) |
and similarly below.
daira
reviewed
Nov 23, 2024
| @@ -53,7 +53,7 @@ class DerivationToolMainnetTests: XCTestCase { | |||
| func testDeriveViewingKeysFromSeed() throws { | |||
| let seedBytes = [UInt8](seedData) | |||
|
|
|||
| let spendingKey = try derivationTool.deriveUnifiedSpendingKey(seed: seedBytes, accountIndex: 0) | |||
| let spendingKey = try derivationTool.deriveUnifiedSpendingKey(seed: seedBytes, account: Account(0)) | |||
There was a problem hiding this comment.
Suggested change
| let spendingKey = try derivationTool.deriveUnifiedSpendingKey(seed: seedBytes, account: Account(0)) | |
| let spendingKey = try derivationTool.deriveUnifiedSpendingKey(seed: seedBytes, accountIndex: Zip32AccountIndex(0)) |
and similarly below.
daira
reviewed
Nov 23, 2024
| @@ -86,7 +86,7 @@ class TestCoordinator { | |||
|
|
|||
| self.spendingKey = try derivationTool.deriveUnifiedSpendingKey( | |||
| seed: Environment.seedBytes, | |||
| accountIndex: 0 | |||
| account: Account(0) | |||
There was a problem hiding this comment.
Suggested change
| account: Account(0) | |
| accountIndex: Zip32AccountIndex(0) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1511
DerivationTooluses accountIndex: Int32from_account_id/to_account_idrefactored to AccountId(Int)This code review checklist is intended to serve as a starting point for the author and reviewer, although it may not be appropriate for all types of changes (e.g. fixing a spelling typo in documentation). For more in-depth discussion of how we think about code review, please see Code Review Guidelines.
Author
Reviewer