fix: string slice panic & automated smart contract tests #76
Conversation
| @@ -35,7 +35,7 @@ jobs: | |||
| rust: nightly | |||
| - name: "Tests" | |||
| cmd: nextest | |||
| args: run --workspace --all-features --features cacao --retries 3 | |||
| args: run --workspace --all-features | |||
There was a problem hiding this comment.
--all-features implies --features cacao
--retries doesn't seem to be necessary in latest version
| @@ -4,7 +4,7 @@ version = "0.1.0" | |||
| edition = "2021" | |||
|
|
|||
| [dependencies] | |||
| relay_rpc = { path = "../relay_rpc" } | |||
| relay_rpc = { path = "../relay_rpc", features = ["cacao"] } | |||
There was a problem hiding this comment.
This doesn't actually compile without this; forgot in previous PRs. Wasn't caught because CI here always sets this feature.
relay_rpc/src/lib.rs
Outdated
| @@ -1,3 +1,5 @@ | |||
| #![deny(clippy::indexing_slicing)] | |||
There was a problem hiding this comment.
Slices/indexing can be unsafe generally and easy to overlook for panics. People can #[allow(clippy::indexing_slicing)] for each instance to bypass if confident it won't panic.
There was a problem hiding this comment.
this can also be set on the manifest so it's applied to the entire codebase like so:
[lints.clippy]
indexing_slicing = "deny"
| @@ -149,12 +149,17 @@ pub trait VerifyableClaims: Serialize + DeserializeOwned { | |||
|
|
|||
| // Decode header. | |||
| data_encoding::BASE64URL_NOPAD | |||
| .decode_mut(header.as_bytes(), &mut output[..header_len]) | |||
| .decode_mut( | |||
There was a problem hiding this comment.
I think these are all safe slices already, but replacing with error catching instead of clippy allow because we are already using Results and there seems to be 1 shared Error variant for all cases here.
I would prefer if we had different error variants for each case, and separating the client errors from the server errors.
chris13524
force-pushed
the
fix/string-slice-panic
branch
from
72f9311 to
a9ed09e
Compare
chris13524
force-pushed
the
fix/string-slice-panic
branch
from
a9ed09e to
4424b74
Compare
| .wait_with_output() | ||
| .await | ||
| .unwrap(); | ||
| println!("forge status: {:?}", output.status); |
There was a problem hiding this comment.
were these prints intended to be there?
There was a problem hiding this comment.
Yeah these are helpful if this tool fails to start for some reason. They don't get printed unless the test fails.
Description
Fixes a possible panic when attempting to slice empty array response. This can be triggered by providing an invalid contract address to validate.
Also add automated tests for EIP-1271.
Also fix potential panics in EIP-191 signature verification, and globally deny array slicing.
How Has This Been Tested?
New tests
Due Diligence