Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable integration tests + GitHub Actions Service for Tendermint node #183

Merged
merged 2 commits into from
Mar 16, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,32 @@ jobs:
- uses: actions-rs/cargo@v1
with:
command: build
args: --all --all-targets
args: --workspace --all-targets
greg-szabo marked this conversation as resolved.
Show resolved Hide resolved
- uses: actions-rs/cargo@v1
with:
command: test
args: --all-features --no-fail-fast

test-integration-ignored:
runs-on: ubuntu-latest
services:
tendermint:
image: interchainio/tendermint
ports:
- 26656:26656
- 26657:26657
- 26660:26660
steps:
- uses: actions/checkout@v2
- uses: actions-rs/toolchain@v1
with:
toolchain: stable
override: true
- uses: actions-rs/cargo@v1
with:
command: test
args: -p tendermint --test integration --no-fail-fast -- --ignored
greg-szabo marked this conversation as resolved.
Show resolved Hide resolved

test-nightly-coverage:
runs-on: ubuntu-latest
steps:
Expand Down
2 changes: 1 addition & 1 deletion CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
Dependencies
- Update to bytes `0.5` and amino_rs `0.5`.
- Tokens for amino_rs are now fully non-conflicting with prost. Allowing both to be used together

- Made RPC type values optional for full compatibility with tendermint-go: `abci_info`, `abci_query` [#120]
greg-szabo marked this conversation as resolved.
Show resolved Hide resolved

## [0.11.0] (2019-12-11)

Expand Down
2 changes: 1 addition & 1 deletion tendermint/src/genesis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@ pub struct Genesis<AppState = serde_json::Value> {
pub app_hash: Option<Hash>,

/// App state
pub app_state: AppState,
pub app_state: Option<AppState>,
}
31 changes: 24 additions & 7 deletions tendermint/src/rpc/endpoint/abci_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,41 +27,58 @@ impl rpc::Response for Response {}

/// ABCI information
#[derive(Clone, Debug, Deserialize, Serialize)]
#[serde(default)]
pub struct AbciInfo {
/// Name of the application
pub data: String,

/// Version
#[serde(skip_serializing_if = "Option::is_none")]
greg-szabo marked this conversation as resolved.
Show resolved Hide resolved
pub version: Option<String>,

/// Last block height
pub last_block_height: block::Height,
#[serde(skip_serializing_if = "Option::is_none")]
pub last_block_height: Option<block::Height>,
greg-szabo marked this conversation as resolved.
Show resolved Hide resolved

/// Last app hash for the block
#[serde(
serialize_with = "serialize_app_hash",
deserialize_with = "parse_app_hash"
deserialize_with = "parse_app_hash",
skip_serializing_if = "Option::is_none"
)]
pub last_block_app_hash: Hash,
pub last_block_app_hash: Option<Hash>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above: This can only be empty on genesis, right? Maybe add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, abci_info doesn't print this over RPC.

}

/// Parse Base64-encoded app hash
pub(crate) fn parse_app_hash<'de, D>(deserializer: D) -> Result<Hash, D::Error>
pub(crate) fn parse_app_hash<'de, D>(deserializer: D) -> Result<Option<Hash>, D::Error>
where
D: Deserializer<'de>,
{
let bytes = base64::decode(String::deserialize(deserializer)?.as_bytes())
.map_err(|e| D::Error::custom(format!("{}", e)))?;

Hash::new(hash::Algorithm::Sha256, &bytes).map_err(|e| D::Error::custom(format!("{}", e)))
Hash::new(hash::Algorithm::Sha256, &bytes)
.map(Some)
.map_err(|e| D::Error::custom(format!("{}", e)))
greg-szabo marked this conversation as resolved.
Show resolved Hide resolved
}

/// Serialize Base64-encoded app hash
pub(crate) fn serialize_app_hash<S>(hash: &Hash, serializer: S) -> Result<S::Ok, S::Error>
pub(crate) fn serialize_app_hash<S>(hash: &Option<Hash>, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
String::from_utf8(base64::encode(hash.as_bytes()))
String::from_utf8(base64::encode(hash.unwrap().as_bytes()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this panic if hash is None?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting point. The normal execution in serde is that if the parameter is missing or None and you have the Default trait implemented, then this serializer function is not run. The Default implementation takes over and assumes whatever value is defined there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is definitely something fishy about this 🤔 I think the method should err if hash is None which is possible. At least the type system allows that even if used as intended this won't happen.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why we have the skip_serializing_if = "Option::is_none" added. If hash is None, serde will not call the function.

.unwrap()
.serialize(serializer)
}

impl Default for AbciInfo {
fn default() -> Self {
AbciInfo {
data: "".to_string(),
version: None,
last_block_height: None,
last_block_app_hash: None,
}
}
}
greg-szabo marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 2 additions & 0 deletions tendermint/src/rpc/endpoint/abci_query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use serde::{Deserialize, Serialize};
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
pub struct Request {
/// Path to the data
#[serde(skip_serializing_if = "Option::is_none")]
path: Option<Path>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprising that this can be empty. But yeah, in JSON everything seems possible :-D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the Tendermint Go code that's surprising in most of these cases. Some of their definitions don't seem to make sense to me, but if we want to be compatible with a full node, we have to follow them.


/// Data to query
Expand All @@ -20,6 +21,7 @@ pub struct Request {
data: Vec<u8>,

/// Block height
#[serde(skip_serializing_if = "Option::is_none")]
height: Option<block::Height>,

/// Include proof in response
Expand Down
2 changes: 1 addition & 1 deletion tendermint/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ mod rpc {
async fn abci_info() {
let abci_info = localhost_rpc_client().abci_info().await.unwrap();

assert_eq!(&abci_info.data, "GaiaApp");
assert_eq!(&abci_info.data, "{\"size\":0}");
}

/// `/abci_query` endpoint
Expand Down
2 changes: 1 addition & 1 deletion tendermint/tests/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ mod endpoints {
.response;

assert_eq!(response.data.as_str(), EXAMPLE_APP);
assert_eq!(response.last_block_height.value(), 488_120);
assert_eq!(response.last_block_height.unwrap().value(), 488_120);
}

#[test]
Expand Down