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

asi-migration preparation #50

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

asi-migration preparation #50

wants to merge 7 commits into from

Conversation

MissingNO57
Copy link

@MissingNO57 MissingNO57 commented Apr 18, 2024

  • Renamed defaults atestfet/afet to atestasi/aasi
  • Fixed unnecessary clonning
  • Removed unused contract_addr_human from State
  • Implemented migration
  • Added wipe_all_roles_from_storage from access_control

@MissingNO57 MissingNO57 requested a review from pbukva April 18, 2024 15:32
@@ -24,7 +24,7 @@ use crate::msg::{ExecuteMsg, InstantiateMsg, QueryMsg, Uint128};
use crate::state::{config_read, State};

pub const DEFAULT_OWNER: &str = "Owner";
pub const DEFAULT_DENUM: &str = "atestfet";
Copy link
Author

Choose a reason for hiding this comment

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

Typo?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes typo, can be fixed

use cosmwasm_std::{entry_point, DepsMut, Env, Response, StdError, StdResult};

#[cfg_attr(not(feature = "library"), entry_point)]
pub fn migrate(deps: DepsMut, env: Env, msg: MigrateMsg) -> StdResult<Response> {
Copy link
Author

Choose a reason for hiding this comment

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

For some reason you can't have MessageInfo which contains sender and sent funds during migration

supply: supply.unwrap_or_else(|_| Uint128::zero()),
fees_accrued: Uint128::zero(),
supply,
fees_accrued,
next_swap_id: msg.next_swap_id,
sealed_reverse_swap_id: 0,
Copy link
Author

@MissingNO57 MissingNO57 Apr 19, 2024

Choose a reason for hiding this comment

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

sealed_reverse_swap_id looks unused, can it be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep it.
I'm not quite seure after these yearsm, but something tells me that was unfinished idea, where some service was suposed to check craop out of evrything, and up to specific reserse_swap_id and if everythign check out, then set the sealed_reverse_swap_id to that reserse_swap_id value, what would mean that all swap ids up to that are 101% checked and so irreversible (non-refundable, etc. ...).

I think it was not implemented, and this is just remnant of that idea.

Copy link
Author

Choose a reason for hiding this comment

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

I am keeping it, but it will just be always 0. It was always 0 anyway.

Ok(Response::default())
}

pub fn initialise_contract_state(
Copy link
Author

@MissingNO57 MissingNO57 Apr 19, 2024

Choose a reason for hiding this comment

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

Maybe it is ok because this migration will be one-time thing, but this means that during migration we will have to set all parameters to contract manually and it will rewrite the original contract State.

One solution would be to keep the legacy state Struct definition, load legacy state value with it and rewrite only parameters we want to change. Or make it to change only denom.

Maybe my implementation is unnecessary too general.

Comment on lines 19 to 21
re_init.supply,
re_init.relay_eon,
re_init.fees_accrued,
Copy link
Contributor

Choose a reason for hiding this comment

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

These 3 attributes should be simply read from the current contract state (= no change in their value).
Though we can query those values from the contract and before initialisation and use them in this re_init message, but it kind of feels dangerous, opening the gate to possible mishaps (wrong copy-paste, ors omething like that).

Copy link
Author

@MissingNO57 MissingNO57 Apr 29, 2024

Choose a reason for hiding this comment

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

We are be able to read it from contract state only if legacy contract state is compatible with new contract state or if we keep the original contract state definition for migration sake.

Copy link
Author

Choose a reason for hiding this comment

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

I've added legacy state handling

Comment on lines 170 to 172
pub supply: Uint128,
pub relay_eon: u64,
pub fees_accrued: Uint128,
Copy link
Contributor

Choose a reason for hiding this comment

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

@MissingNO57 MissingNO57 requested a review from pbukva April 29, 2024 14:36
@MissingNO57 MissingNO57 marked this pull request as ready for review April 29, 2024 14:37
Copy link
Contributor

@pbukva pbukva left a comment

Choose a reason for hiding this comment

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

LGTM

@MissingNO57 MissingNO57 changed the title [WiP] asi-migration preparation asi-migration preparation Apr 30, 2024
@MissingNO57 MissingNO57 requested a review from lrahmani April 30, 2024 09:58
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