Conversation
Updated the remote fetching implementation to use reqwest instead of curl, improving compatibility and simplifying error handling. Adjusted Cargo.toml to reflect the change in dependencies and removed obsolete curl configurations from Cargo.lock. This change enhances the overall performance and maintainability of the code.
…o.toml Added new dependencies including foreign-types, hyper-tls, native-tls, and openssl-related packages to Cargo.lock. Updated reqwest configuration in Cargo.toml to disable default features and enable blocking and native-tls features for improved functionality.
…features in Cargo.toml Removed several obsolete dependencies from Cargo.lock to streamline the project. Updated reqwest configuration in Cargo.toml to disable default features while enabling json and native-tls for enhanced functionality.
| [dependencies.reqwest] | ||
| version = "0.13" | ||
| features = [ "json" ] | ||
| default-features = false |
There was a problem hiding this comment.
Is this needed? If not can you revert (i.e. remove this line)
There was a problem hiding this comment.
It is and the reason is that with version 0.13 it installs by default some aws-ls-* libraries that break android. In this way, we avoid that. I tried several ways but android keeps crashing.
There was a problem hiding this comment.
Is it possible to make this a target-dependent exception like we have inparameters/Cargo.toml? It's hard to test on all possible platforms and we want to avoid breaking changes
There was a problem hiding this comment.
I will take a look if it is possible to do
There was a problem hiding this comment.
Update: this seems impossible: rust-lang/cargo#1197
Let's take the risk, if some client software depending on snarkVM breaks then we can figure out a resolution
There was a problem hiding this comment.
Ok sounds good. Let's wait on @iamalwaysuncomfortable review and then I guess we can proceed
There was a problem hiding this comment.
Update: this seems impossible: rust-lang/cargo#1197 Let's take the risk, if some client software depending on snarkVM breaks then we can figure out a resolution
@vicsn @fedpre It's impossible not with resolver v2, we do it on the very next line of this Cargo.toml.
[target.'cfg(target_arch="wasm32")'.dependencies.ureq]
workspace = true
features = [ "json", "gzip" ]
optional = true
[target.'cfg(not(target_arch="wasm32"))'.dependencies.ureq]
workspace = true
features = [ "json", "gzip", "rustls" ]
optional = trueTherefore this should work:
[target.'cfg(any(target_os = "android", target_os = "ios"))'.dependencies.reqwest]
version = "0.13"
default-features = false
features = [ "json", "native-tls" ]
optional = true
[target.'cfg(not(any(target_os = "android", target_os = "ios")))'.dependencies.reqwest]
version = "0.13"
default-features = false
features = [ "json" ]
optional = true|
@vicsn is this still mergeable? |
| [dependencies.reqwest] | ||
| version = "0.13" | ||
| features = [ "json" ] | ||
| default-features = false |
There was a problem hiding this comment.
Update: this seems impossible: rust-lang/cargo#1197 Let's take the risk, if some client software depending on snarkVM breaks then we can figure out a resolution
@vicsn @fedpre It's impossible not with resolver v2, we do it on the very next line of this Cargo.toml.
[target.'cfg(target_arch="wasm32")'.dependencies.ureq]
workspace = true
features = [ "json", "gzip" ]
optional = true
[target.'cfg(not(target_arch="wasm32"))'.dependencies.ureq]
workspace = true
features = [ "json", "gzip", "rustls" ]
optional = trueTherefore this should work:
[target.'cfg(any(target_os = "android", target_os = "ios"))'.dependencies.reqwest]
version = "0.13"
default-features = false
features = [ "json", "native-tls" ]
optional = true
[target.'cfg(not(any(target_os = "android", target_os = "ios")))'.dependencies.reqwest]
version = "0.13"
default-features = false
features = [ "json" ]
optional = true| default-features = false | ||
| features = [ "json", "native-tls" ] |
There was a problem hiding this comment.
Native-tls will break wasm targets and have easily the capability of breaking multiple other Rust implementations who are using rust-tls as this will enforce native-tls.
To avoid downstream breakage, the change should be this:
[target.'cfg(any(target_os = "android", target_os = "ios"))'.dependencies.reqwest]
version = "0.13"
default-features = false
features = [ "json", "native-tls" ]
optional = true
[target.'cfg(not(any(target_os = "android", target_os = "ios")))'.dependencies.reqwest]
version = "0.13"
default-features = false
features = [ "json" ]
optional = true| [target.'cfg(all(not(target_family = "wasm"), not(target_env = "sgx")))'.dependencies.reqwest] | ||
| version = "0.13" | ||
| default-features = false | ||
| features = [ "blocking", "native-tls" ] |
There was a problem hiding this comment.
Again native-tls would break wasm and easily break other could tls stacks as it'd enforce native-tls usage. Just target the mobile platforms specifically.
| [target.'cfg(all(not(target_family = "wasm"), not(target_env = "sgx")))'.dependencies.reqwest] | |
| version = "0.13" | |
| default-features = false | |
| features = [ "blocking", "native-tls" ] | |
| [target.'cfg(all( | |
| any(target_os = "android", target_os = "ios"), | |
| not(target_family = "wasm"), | |
| not(target_env = "sgx") | |
| ))'.dependencies.reqwest] | |
| version = "0.13" | |
| default-features = false | |
| features = [ "blocking", "native-tls" ] | |
| [target.'cfg(all( | |
| not(any(target_os = "android", target_os = "ios")), | |
| not(target_family = "wasm"), | |
| not(target_env = "sgx") | |
| ))'.dependencies.reqwest] | |
| version = "0.13" | |
| default-features = false | |
| features = [ "blocking" ] |
Motivation
Updated the remote fetching implementation to use reqwest instead of curl to avoid issues with CA certificate. This changes uses the
native-tslfromreqwuestwithout any additional step.Test Plan
Tested on both iOS and Android devices and the change works.
Backwards compatibility
Please review backwards compatibility. Does any functionality need to be guarded by an existing or new
ConsensusVersion?