Migrate to first pre-release of FFI 0.9.0 #1446
Conversation
| @@ -56,6 +56,7 @@ actor CompactBlockProcessor { | |||
| let saplingParamsSourceURL: SaplingParamsSourceURL | |||
| let fsBlockCacheRoot: URL | |||
| let dataDb: URL | |||
| let torDir: URL | |||
There was a problem hiding this comment.
I feel like there are now too many of these; I originally was passing this through to ZcashRustBackend, but now have a separate TorClient that only the SDKSynchronizer needs to know about. But I didn't have time to figure out which of these could be reverted.
There was a problem hiding this comment.
Agreed but not high-prio to deal with it right now I believe. We can file a followup ticket to maybe pass a URLConfiguration or something like that.
| @@ -823,33 +847,33 @@ struct ZcashRustBackend: ZcashRustBackendWelding { | |||
| } | |||
|
|
|||
| private extension ZcashRustBackend { | |||
| static func enableTracing() { | |||
| zcashlc_init_on_load(false) | |||
There was a problem hiding this comment.
The old code here was treating the enableTracing argument to ZcashRustBackend as enabling Rust logging, rather than enabling trace-level logging. The changes I've made so far in this PR have preserved that, but only because OSLogger.LogLevel has no trace level.
| case .info: | ||
| rustLogging = RustLogging.info | ||
| case .event: | ||
| rustLogging = RustLogging.info |
There was a problem hiding this comment.
Rust has no EVENT level, so I've mapped that to INFO.
There was a problem hiding this comment.
This could be shortened to
case .info, .event:
rustLogging = RustLogging.info
str4d
force-pushed
the
rust-logging-and-exchange-rates
branch
from
ea54bf1
to
fca9729
Compare
| @@ -9,6 +9,21 @@ | |||
| import Foundation | |||
| import libzcashlc | |||
|
|
|||
| enum RustLogging: String { | |||
| /// The logs are completely disabled. | |||
| case off = "off" | |||
There was a problem hiding this comment.
When enum is defined as String (enum RustLogging: String) the exact value should be omitted unless we want them to be a different value which is not this case. So all cases must be in format case off instead of case off = "off"
There was a problem hiding this comment.
Ah, will the string discriminant be taken from the enum variant names in that case? What matters here is they exactly match what tracing expects on the Rust side.
There was a problem hiding this comment.
Yes, as long as your expected strings match the case names. Which is the case I believe when I reviewed it. The assign of string is used when the name doesn't match the value, usually for key-value scenarios. For example json en-de/coding, example:
enum JsonKeys: String {
case name = "first-name"
case surname = "second-name"
}Dashes and other special characters make it "impossible" to be determined by the case name itself. If json keys are simple ones, value and :String is omitted and case names are used automatically, example:
enum JsonKeys {
case name // matches 'name' in the json
case surname // matches 'surname' in the json
}
| rustLogging = RustLogging.debug | ||
| case .info: | ||
| rustLogging = RustLogging.info | ||
| case .event: |
There was a problem hiding this comment.
Same here, case .info, .event: deals with both
| private let fileManager = FileManager() | ||
| private let runtime: OpaquePointer | ||
|
|
||
| init(torDir: URL) async throws { | ||
| // Ensure that the directory exists. | ||
| if !fileManager.fileExists(atPath: torDir.path) { | ||
| do { | ||
| try fileManager.createDirectory(at: torDir, withIntermediateDirectories: true) | ||
| } catch { | ||
| throw ZcashError.blockRepositoryCreateBlocksCacheDirectory(torDir, error) | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think it's necessary to hold an instance of FileManager for a lifetime of the TorClient as it's used only in the init. Also it not defined as a custom file manager so FileManager.default is sufficient enough for 2 calls or define let fileManager = FileManager() in the init and reuse it.
| @@ -309,6 +309,9 @@ public protocol Synchronizer: AnyObject { | |||
| /// - Returns: `AccountBalance`, struct that holds sapling and unshielded balances or `nil` when no account is associated with `accountIndex` | |||
| func getAccountBalance(accountIndex: Int) async throws -> AccountBalance? | |||
|
|
|||
| /// Fetches the latest ZEC-USD exchange rate. | |||
| func getExchangeRateUSD() async throws -> Float64 | |||
There was a problem hiding this comment.
One note about this proposed SDK method is that it always does a live lookup and has no caching (the caller, e.g. Zashi, would need to cache as desired). Additionally, this method is somewhat slow (a few seconds), particularly on first usage (10+ seconds).
@LukasKorba another approach we could take here is to have a cached value somewhere, and a refreshExchangeRateUSD() method that updates it (so the SDK tracks the last-observed exchange rate). Either approach uses the same backend Rust code, it's just how this gets presented to the SDK user. Thoughts (and suggestions of how to expose if so)?
Includes: - Initialization changes to enable log filter customization. We now connect the Rust log level to the Swift log level, and always run other Rust initialization steps. - Fetching the ZEC-USD exchange rate over Tor.
str4d
force-pushed
the
rust-logging-and-exchange-rates
branch
from
fca9729
to
929f198
Compare
|
Force-pushed to address comments, and replace |
Includes:
Depends on Electric-Coin-Company/zcash-light-client-ffi#142.
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