-
Notifications
You must be signed in to change notification settings - Fork 8
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
refactor instantiation logic #94
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just some comments about the overall design.
Will review the SDK PR next.
pub fn get_babylon_sdk_params(querier: &QuerierWrapper) -> Result<Params, StdError> { | ||
let params = querier.query_grpc(QUERY_PARAMS_PATH.to_owned(), Binary::new("".into()))?; | ||
let params = QueryParamsResponse::from(params).params; | ||
Ok(params) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach is not without merit, but it would be much simpler IMO to just pass the needed params during instantiation. The contracts have a clear dependencies, so it's just a matter of instantiating them in the right order and pass required parameters (other contracts' addresses) between them at instantiation.
pub babylon: Addr, | ||
pub staking: Addr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just keep these, and pass / set them during instantiation.
pub fn get_activated_height(querier: &QuerierWrapper) -> StdResult<u64> { | ||
let params = get_babylon_sdk_params(querier)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This is good for encapsulation, but bad for performance. In any case, since you already have it why not pass the entire Config
by reference instead?
No description provided.