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

chore: Review fixes #458

Merged
merged 3 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
46 changes: 43 additions & 3 deletions sylvia-derive/src/contract/communication/reply.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,36 @@
use crate::types::msg_variant::{MsgVariant, MsgVariants};
use crate::utils::emit_turbofish;

const NUMBER_OF_DATA_FIELDS: usize = 1;
const NO_ALLOWED_DATA_FIELDS: usize = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

NO stands for number here? If so it's quite misleading in this context

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ye, number of. It is a bit if you don't check the type. I will revert this change.

const NO_ALLOWED_RAW_PAYLOAD_FIELDS: usize = 1;

/// Make sure that there are no additional parameters between ones marked
/// with `sv::data` and `sv::payload` and after the one marked with `sv::payload`.
fn assert_no_redundant_params(payload: &[&MsgField]) {
let payload_param = payload.iter().enumerate().find(|(_, field)| {
ParsedSylviaAttributes::new(field.attrs().iter())
.payload
.is_some()
});

if payload.len() == NO_ALLOWED_RAW_PAYLOAD_FIELDS {
return;
}

let Some((index, payload_param)) = payload_param else {

Check warning on line 30 in sylvia-derive/src/contract/communication/reply.rs

View check run for this annotation

Codecov / codecov/patch

sylvia-derive/src/contract/communication/reply.rs#L30

Added line #L30 was not covered by tests
return;
};

if index == 0 {
emit_error!(payload[1].name().span(), "Redundant payload parameter.";
note = payload_param.name().span() => "Expected no parameters after the parameter marked with `#[sv::payload(raw)]`."

Check warning on line 36 in sylvia-derive/src/contract/communication/reply.rs

View check run for this annotation

Codecov / codecov/patch

sylvia-derive/src/contract/communication/reply.rs#L35-L36

Added lines #L35 - L36 were not covered by tests
)
} else {
emit_error!(payload[0].name().span(), "Redundant payload parameter.";
note = payload_param.name().span() => "Expected no parameters between the parameter marked with `#[sv::data]` and `#[sv::payload(raw)]`."

Check warning on line 40 in sylvia-derive/src/contract/communication/reply.rs

View check run for this annotation

Codecov / codecov/patch

sylvia-derive/src/contract/communication/reply.rs#L39-L40

Added lines #L39 - L40 were not covered by tests
)
}
}

pub struct Reply<'a> {
source: &'a ItemImpl,
Expand Down Expand Up @@ -208,10 +237,11 @@
variant.validate_fields_attributes();
let payload = variant.fields().iter();
let payload = if data.is_some() || variant.msg_attr().reply_on() != ReplyOn::Success {
payload.skip(NUMBER_OF_DATA_FIELDS).collect::<Vec<_>>()
payload.skip(NO_ALLOWED_DATA_FIELDS).collect::<Vec<_>>()
} else {
payload.collect::<Vec<_>>()
};
assert_no_redundant_params(&payload);

Check warning on line 244 in sylvia-derive/src/contract/communication/reply.rs

View check run for this annotation

Codecov / codecov/patch

sylvia-derive/src/contract/communication/reply.rs#L244

Added line #L244 was not covered by tests
let method_name = variant.function_name();
let reply_on = variant.msg_attr().reply_on();

Expand Down Expand Up @@ -523,8 +553,18 @@
let sylvia = crate_module();
let data = ParsedSylviaAttributes::new(self.attrs().iter()).data;
let missing_data_err = "Missing reply data field.";
let transaction_id = quote! {
env
.transaction
.as_ref()
.map(|tx| format!("{}", &tx.index))
.unwrap_or_else(|| "Missing".to_string())
};
let invalid_reply_data_err = quote! {
format! {"Invalid reply data: {}\nSerde error while deserializing {}", data, err}
format! {"Invalid reply data at block height: {}, transaction id: {}.\nSerde error while deserializing {}",
env.block.height,
#transaction_id,
err}
};
let execute_data_deserialization = quote! {
let deserialized_data =
Expand Down
15 changes: 11 additions & 4 deletions sylvia/tests/reply_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,9 +336,11 @@ fn data_opt() {
.call(&owner)
.unwrap_err();
assert_eq!(
err,
StdError::generic_err("Invalid reply data: SW52YWxpZERhdGE=\nSerde error while deserializing Error parsing into type alloc::string::String: Invalid type").into()
);
err,
StdError::generic_err(
"Invalid reply data at block height: 12345, transaction id: 0.\nSerde error while deserializing Error parsing into type alloc::string::String: Invalid type"
).into()
);

contract
.send_message_expecting_data(data.clone(), DATA_OPT_REPLY_ID)
Expand Down Expand Up @@ -377,7 +379,12 @@ fn data() {
.send_message_expecting_data(invalid_data, DATA_REPLY_ID)
.call(&owner)
.unwrap_err();
assert_eq!(err, StdError::generic_err("Invalid reply data: SW52YWxpZERhdGE=\nSerde error while deserializing Error parsing into type alloc::string::String: Invalid type").into());
assert_eq!(
err,
StdError::generic_err(
"Invalid reply data at block height: 12345, transaction id: 0.\nSerde error while deserializing Error parsing into type alloc::string::String: Invalid type"
).into()
);

contract
.send_message_expecting_data(data, DATA_REPLY_ID)
Expand Down
48 changes: 46 additions & 2 deletions sylvia/tests/ui/method_signature/reply.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#![allow(unused_imports)]
#![allow(unused_imports, unused_variables)]
use sylvia::ctx::{InstantiateCtx, ReplyCtx};
use sylvia::cw_std::{Binary, Response, StdResult};
use sylvia::cw_std::{Addr, Binary, Response, StdResult};

pub mod mismatched_params {
use super::*;
Expand Down Expand Up @@ -81,4 +81,48 @@ pub mod mismatched_param_arity {
}
}

pub mod redundant_params {
use super::*;

pub struct Contract {}

#[sylvia::contract]
#[sv::features(replies)]
impl Contract {
pub const fn new() -> Self {
Self {}
}

#[sv::msg(instantiate)]
fn instantiate(&self, _ctx: InstantiateCtx) -> StdResult<Response> {
Ok(Response::new())
}

#[sv::msg(reply, reply_on=success)]
fn first_reply(
&self,
_ctx: ReplyCtx,
#[sv::data(opt, raw)] _data: Option<Binary>,
redundant_between1: u32,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add few more combinations:

  1. What would happen if there is an additional argument before sv::data?
  2. What happen if we use #[sv::data(opt, raw)] on ctx: ReplyCtx?
  3. What happen if we use #[sv::payload(raw)] on ctx: ReplyCtx?
  4. What if we skip ctx: ReplyCtx?
  5. Can we use #[sv::data(raw)] after #[sv::payload(raw)]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Missing payload parameter. - This might cause some weird behavior due to deserialization to empty tuple.

Good points.
For 2 and 3 nothing would happen as context types are skipped during parsing.
For 4 as in case of every message type it would fail with Rust compile error that context types couldn't be converted to whatever parameter is defined first. We are unable to check if the second parameter in the method signature is ReplyCtx as this would make usage of aliases impossible or very restricted and in the end wrong type could still be passed there.

Added test cases for the rest.

redudnant_between2: String,
#[sv::payload(raw)] param: String,
redundant_after1: Binary,
redundant_after2: Addr,
) -> StdResult<Response> {
Ok(Response::new())
}

#[sv::msg(reply, reply_on=success)]
fn second_reply(
&self,
_ctx: ReplyCtx,
#[sv::data(opt, raw)] _data: Option<Binary>,
#[sv::payload(raw)] param: String,
redundant: Binary,
) -> StdResult<Response> {
Ok(Response::new())
}
}
}

fn main() {}
18 changes: 18 additions & 0 deletions sylvia/tests/ui/method_signature/reply.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,21 @@ error: Mismatched quantity of method parameters.
|
62 | fn first_reply(
| ^^^^^^^^^^^

error: Redundant payload parameter.

= note: Expected no parameters between the parameter marked with `#[sv::data]` and `#[sv::payload(raw)]`.

--> tests/ui/method_signature/reply.rs:106:13
|
106 | redundant_between1: u32,
| ^^^^^^^^^^^^^^^^^^

error: Redundant payload parameter.

= note: Expected no parameters after the parameter marked with `#[sv::payload(raw)]`.

--> tests/ui/method_signature/reply.rs:121:13
|
121 | redundant: Binary,
| ^^^^^^^^^
Loading