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

Remove proxy and manager contracts and packages #444

Merged

Conversation

Kayanski
Copy link
Contributor

@Kayanski Kayanski commented Sep 9, 2024

This PR aims at removing the manager and proxy contracts completely from the repository

Checklist

  • CI is green.
  • Changelog updated.

@Kayanski Kayanski marked this pull request as draft September 9, 2024 15:09
Copy link

cloudflare-workers-and-pages bot commented Sep 9, 2024

Deploying abstract-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: a6b08a0
Status:⚡️  Build in progress...

View logs

Copy link
Collaborator

@Buckram123 Buckram123 left a comment

Choose a reason for hiding this comment

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

I'm not sure why we have msgs, types and responses in separate files for the account? Is it something we want to follow with other contracts as well?

framework/packages/abstract-std/src/account/state.rs Outdated Show resolved Hide resolved
framework/packages/abstract-std/src/account/state.rs Outdated Show resolved Hide resolved
@CyberHoward
Copy link
Contributor

I'm not sure why we have msgs, types and responses in separate files for the account? Is it something we want to follow with other contracts as well?

I kind of like the current convention we have as well.

pub mod state {
   // Constants 
   
   // Types 
}

// Messages

// Responses

@CyberHoward CyberHoward mentioned this pull request Sep 12, 2024
2 tasks
Comment on lines +11 to +31
/// Add the message's data to the response
pub fn forward_response_data(result: Reply) -> AccountResult {
// get the result from the reply
let res = result.result.into_result().map_err(StdError::generic_err)?;

// log and add data if needed
let resp = if let Some(data) = res.data {
AccountResponse::new(
"forward_response_data_reply",
vec![("response_data", "true")],
)
.set_data(data)
} else {
AccountResponse::new(
"forward_response_data_reply",
vec![("response_data", "false")],
)
};

Ok(resp)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@CyberHoward do we even need this method in cosmwasm 2? Won't it be possible to get data from the new field of SubMsgResponse: https://docs.rs/cosmwasm-std/latest/cosmwasm_std/struct.SubMsgResponse.html#structfield.msg_responses? Let's mark this method as deprecated for reminder as well if so

Copy link
Contributor Author

@Kayanski Kayanski Sep 12, 2024

Choose a reason for hiding this comment

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

I think it's the same behavior here, because it's still a nested execution.
We could have a test for thoses two cases, but I don't think it's deprecated yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Asked for clarification here : CosmWasm/cosmwasm#2253

Copy link
Contributor

Choose a reason for hiding this comment

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

@CyberHoward do we even need this method in cosmwasm 2? Won't it be possible to get data from the new field of SubMsgResponse: https://docs.rs/cosmwasm-std/latest/cosmwasm_std/struct.SubMsgResponse.html#structfield.msg_responses? Let's mark this method as deprecated for reminder as well if so

Yeah I removed the allow(deprecated)

But we should do this in a different PR imo.

@CyberHoward CyberHoward marked this pull request as ready for review September 12, 2024 13:16
@CyberHoward CyberHoward merged commit d3f3ed1 into develop/v2 Sep 12, 2024
1 check passed
@CyberHoward CyberHoward deleted the nicolas/abs-513-remove-proxy-and-manager-contractsmods branch September 12, 2024 13:16
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.

3 participants