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

Update/Must use on abstract sdk methods #86

Merged
merged 6 commits into from
Sep 22, 2023

Conversation

Buckram123
Copy link
Collaborator

@Buckram123 Buckram123 commented Sep 19, 2023

This adds warnings on unused AccountAction and unused CosmosMsg from the Executor

  • Helpfully add_message and add_messages takes impl Into<CosmosMsg<T>>, so it shouldn't affect users.
  • Unfortunately add_submessage takes raw SubMsg<T>.

So we still have an issue: we can't implement message wrapper with unused warning for unused SubMsg, because it will require users to cast it using "into" every time

Checklist

  • CI is green.
  • Changelog updated.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Sep 19, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 205ddfd
Status: ✅  Deploy successful!
Preview URL: https://c544bf8d.abstract-docs.pages.dev
Branch Preview URL: https://buckram-abs-194-add-must-use.abstract-docs.pages.dev

View logs

@github-actions github-actions bot added the sdk label Sep 19, 2023
@Kayanski
Copy link
Contributor

It's already something that the sdk provides helpers for simple message mechanisms.
For messages with replies, I think we can assume that developers are more advanced and will not forget to use the message result !

@Kayanski
Copy link
Contributor

Kayanski commented Sep 20, 2023

Shouldn't we have a single way of executing all those actions ?
We could pass the DepositMsgs and AccountAction all to the executor and it would decide what to do with it ?
So that the sdk would always create something you have to pass to the executor and then to the Response::add_messagesfunction ?

@Buckram123
Copy link
Collaborator Author

Shouldn't we have a single way of executing all those actions ? We could pass the DepositMsgs and AccountAction all to the executor and it would decide what to do with it ? So that the sdk would always create something you have to pass to the executor and then to the Response::add_messagesfunction ?

Right now executor executes actions only from the proxy, idk if we want to expand that. DepositMsgs sends funds from this contract to the proxy, so it's not an action from the proxy

Co-authored-by: CyberHoward <88450409+CyberHoward@users.noreply.github.com>
@CyberHoward
Copy link
Contributor

Shouldn't we have a single way of executing all those actions ? We could pass the DepositMsgs and AccountAction all to the executor and it would decide what to do with it ? So that the sdk would always create something you have to pass to the executor and then to the Response::add_messagesfunction ?

Right now executor executes actions only from the proxy, idk if we want to expand that. DepositMsgs sends funds from this contract to the proxy, so it's not an action from the proxy

I think Nicolas brings up a good point. The executor API is kind of confusing but IMO we should stick with it for now. We can look into removing the need for Response construction entirely by having a call stack inside the module struct itself and executing that stack at the end of a contract's execution.

@CyberHoward CyberHoward merged commit 66187bf into main Sep 22, 2023
12 checks passed
@CyberHoward CyberHoward deleted the buckram/abs-194-add-must_use-to-abstract-sdk-calls branch September 22, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants