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: Replies test contract #2306

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kulikthebird
Copy link
Contributor

No description provided.

@kulikthebird kulikthebird force-pushed the tkulik/replies_test_contract branch 5 times, most recently from 157902d to a06d4f1 Compare December 6, 2024 11:30
@kulikthebird kulikthebird marked this pull request as ready for review December 9, 2024 09:35
@kulikthebird kulikthebird self-assigned this Dec 9, 2024
@kulikthebird kulikthebird force-pushed the tkulik/replies_test_contract branch 4 times, most recently from b0360f0 to 99b92a3 Compare December 10, 2024 08:40
Copy link
Collaborator

@chipshort chipshort left a comment

Choose a reason for hiding this comment

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

Nice! Can you add a CI job for this contract (see e.g. contract_burner in .circleci/config.yml)?
That should also reveal some more clippy lints we should fix.

contracts/replier/src/lib.rs Outdated Show resolved Hide resolved
contracts/replier/src/lib.rs Outdated Show resolved Hide resolved
msg: to_json_binary(&next_msg).unwrap(),
funds: vec![],
};
let mut msg_id: u64 = msg.msg_id.into();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it intentional that this uses the parent's msg_id field instead of next_msg.msg_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. Short answer is: yes, it was intentional. I double-checked the test scenarios in wasmd and it seems that it doesn't matter which of those two information is chosen for the reply ID. If we find a valid scenario where this will matter then we can change that in this contract.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My main reason for asking is that it confused me a little while reading the wasmd tests.
For example the first test result is: 0xee, 0x1, 0xee, 0x2, 0xee, 0x3, 0xbb, 0x2, 0xbb, 0x1, 0xee, 0x4, 0xee, 0x5, 0xbb, 0x4, 0xbb, 0x1
There's two times 0xbb, 0x1 in there, once for message 2 and once for message 4, but it's not immediately clear which one was when. But then in this case it makes sense that it would have to be 2 and then 4, since 4 wasn't even executed when the first 0xbb, 0x1 came in, so I guess it's fine.

@kulikthebird kulikthebird force-pushed the tkulik/replies_test_contract branch 3 times, most recently from 02d9c23 to 91079ed Compare December 17, 2024 11:20
@kulikthebird kulikthebird force-pushed the tkulik/replies_test_contract branch from 91079ed to 24c6561 Compare December 17, 2024 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants