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

feat: Move context types to new ctx module (#450) #451

Merged
merged 3 commits into from
Nov 5, 2024

Conversation

jawoznia
Copy link
Collaborator

No description provided.

@jawoznia jawoznia linked an issue Oct 22, 2024 that may be closed by this pull request
Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 38.09524% with 13 lines in your changes missing coverage. Please review.

Project coverage is 72.19%. Comparing base (0df5cf9) to head (403bf47).
Report is 3 commits behind head on feat/replies.

Files with missing lines Patch % Lines
sylvia/src/ctx.rs 35.29% 11 Missing ⚠️
sylvia-derive/src/entry_points.rs 0.00% 1 Missing ⚠️
sylvia/tests/messages_generation.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           feat/replies     #451      +/-   ##
================================================
- Coverage         72.49%   72.19%   -0.30%     
================================================
  Files                62       63       +1     
  Lines              3795     3812      +17     
================================================
+ Hits               2751     2752       +1     
- Misses             1044     1060      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jawoznia jawoznia force-pushed the jawoznia/feat/reply_ctx_2 branch from 13aa5de to b3ff3de Compare October 22, 2024 15:12
Copy link
Collaborator

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

Mostly ok, but I'd like to quickly talk about deprecation order (it's prob right, but I want to double check)

@@ -462,12 +460,13 @@ impl<'a, Contract: ?Sized> AsRef<cosmwasm_std::Addr> for Remote<'a, Contract> {

/// Represantation of `reply` context received in entry point.
#[non_exhaustive]
#[deprecated(
since = "1.3.0",
note = "This type will be replaced with `sylvia::replies::ReplyCtx` in 2.0.0."
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Only thing I am wondering right now - do we want to deprecate this struct or do we want to throw a deprecation warning using #[sv::msg(reply)] without #[sv::features(replies)]? I'm not debating on if this is ok to deprecate this, but which one gives back a better warning message (or even error using the wrong context - however I think that the old context could be in fact used with new messages if the proper From is implemented? Or not? Or maybe both could be? Let's chat about it).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might be used if we initialize the context with empty vectors and set the gas to 0. I'm not sure if the detection which ReplyCtx is used would be possible though.

I went with different approach and moved all the context types to new ctx module.

Copy link
Contributor

@kulikthebird kulikthebird left a comment

Choose a reason for hiding this comment

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

The only comment from me is the one we discussed before - there should be exactly one ReplyCtx that is not deprecated

@jawoznia jawoznia force-pushed the jawoznia/chore/rename_reply_on_failure branch from b6cb2ca to d57d49f Compare October 31, 2024 11:32
Base automatically changed from jawoznia/chore/rename_reply_on_failure to feat/replies October 31, 2024 11:49
@jawoznia jawoznia force-pushed the jawoznia/feat/reply_ctx_2 branch from b3ff3de to 0a3cc71 Compare October 31, 2024 12:26
@jawoznia jawoznia changed the title feat: Add temporary sylvia::replies::ReplyCtx with additional fields (#450) feat: Move context types to new ctx module (#450) Oct 31, 2024
Remove unnecessary `#[allow(deprecated)]` statements.
@jawoznia jawoznia force-pushed the jawoznia/feat/reply_ctx_2 branch from 3eb1ebf to 403bf47 Compare November 4, 2024 16:14
Copy link
Contributor

@kulikthebird kulikthebird left a comment

Choose a reason for hiding this comment

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

LGTM!

@jawoznia jawoznia merged commit c6450bd into feat/replies Nov 5, 2024
7 of 9 checks passed
@jawoznia jawoznia deleted the jawoznia/feat/reply_ctx_2 branch November 5, 2024 08:43
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.

Add temporary ReplyCtx type with additional fields
3 participants