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
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 69 additions & 1 deletion fetch/cosmwasm_contract/src/access_control.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use cosmwasm_std::{Addr, StdError, StdResult, Storage};
use cosmwasm_std::{Addr, Order, StdError, StdResult, Storage};
use cosmwasm_storage::{PrefixedStorage, ReadonlyPrefixedStorage};
use std::str::FromStr;

Expand Down Expand Up @@ -95,3 +95,71 @@ pub fn ac_revoke_role(
addr_roles.remove(role.as_bytes());
Ok(true)
}

pub fn wipe_all_roles_from_storage(storage: &mut dyn Storage) {
let mut roles_storage = PrefixedStorage::new(storage, ACCESS_CONTROL_KEY);
let res: Vec<_> = roles_storage.range(None, None, Order::Ascending).collect();

// Because address is stored as first key it is impossible to clear only specific roles without manually filtering it
for val in res {
roles_storage.remove(&val.0);
}
}

#[cfg(test)]
mod tests {
use super::*;
use cosmwasm_std::testing::mock_dependencies;

#[test]
fn test_wipe_all_roles_from_storage() {
let mut deps = mock_dependencies();

let addresses = vec!["a", "b", "c", "d"];
let roles = vec![
AccessRole::Admin,
AccessRole::Approver,
AccessRole::Monitor,
AccessRole::Relayer,
];

// Add every role and address combination
for address in &addresses {
for role in &roles {
ac_add_role(
deps.as_mut().storage,
&Addr::unchecked(address.to_string()),
role,
)
.unwrap();
}
}

// Every role and address combination is present
for address in &addresses {
for role in &roles {
assert!(ac_have_role(
deps.as_ref().storage,
&Addr::unchecked(address.to_string()),
role
)
.unwrap());
}
}

// Remove all roles
wipe_all_roles_from_storage(deps.as_mut().storage);

// Every role and address combination was removed
for address in &addresses {
for role in &roles {
assert!(!ac_have_role(
deps.as_ref().storage,
&Addr::unchecked(address.to_string()),
role
)
.unwrap());
}
}
}
}
55 changes: 37 additions & 18 deletions fetch/cosmwasm_contract/src/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ use crate::msg::{
};
use crate::state::{config, config_read, refunds_add, refunds_have, State};

pub const DEFAULT_DENOM: &str = "afet";
pub const DEFAULT_DENOM: &str = "aasi";
pub const DEFAULT_RELAY_EON: u64 = 0;
pub const DEFAULT_FEES_ACCRUED: Uint128 = Uint128::zero();

/* ***************************************************
* ************** Initialization *************
Expand All @@ -30,7 +32,29 @@ pub fn instantiate(
info: MessageInfo,
msg: InstantiateMsg,
) -> StdResult<Response> {
let env_message_sender = info.sender;
let supply = amount_from_funds(&info.funds, msg.get_denom()).unwrap_or(Uint128::zero());
initialise_contract_state(
deps.storage,
&env,
&info.sender,
supply,
DEFAULT_RELAY_EON,
DEFAULT_FEES_ACCRUED,
&msg,
)?;

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.

storage: &mut dyn Storage,
env: &Env,
admin: &Addr,
supply: Uint128,
relay_eon: u64,
fees_accrued: Uint128,
msg: &InstantiateMsg,
) -> StdResult<()> {
let current_block_number = env.block.height;

let mut paused_since_block_public_api = msg.paused_since_block.unwrap_or(u64::MAX);
Expand All @@ -39,24 +63,20 @@ pub fn instantiate(
}
let paused_since_block_relayer_api = paused_since_block_public_api;

let contract_addr_human = env.contract.address;

if msg.lower_swap_limit > msg.upper_swap_limit || msg.lower_swap_limit <= msg.swap_fee {
return Err(StdError::generic_err(ERR_SWAP_LIMITS_INCONSISTENT));
}

let denom = msg.denom.unwrap_or_else(|| DEFAULT_DENOM.to_string());
let denom = msg.denom.as_deref().unwrap_or(DEFAULT_DENOM);

ac_add_role(deps.storage, &env_message_sender, &AccessRole::Admin)?;

let supply = amount_from_funds(&info.funds, denom.clone());
ac_add_role(storage, admin, &AccessRole::Admin)?;

let state = State {
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.

relay_eon: 0,
relay_eon,
upper_swap_limit: msg.upper_swap_limit,
lower_swap_limit: msg.lower_swap_limit,
reverse_aggregated_allowance: msg.reverse_aggregated_allowance,
Expand All @@ -65,13 +85,12 @@ pub fn instantiate(
swap_fee: msg.swap_fee,
paused_since_block_public_api,
paused_since_block_relayer_api,
denom,
contract_addr_human, // optimization FIXME(LR) not needed any more (version 0.10.0)
denom: denom.to_string(),
};

config(deps.storage).save(&state)?;
config(storage).save(&state)?;

Ok(Response::default())
Ok(())
}

/* ***************************************************
Expand All @@ -83,7 +102,7 @@ pub fn execute(deps: DepsMut, env: Env, info: MessageInfo, msg: ExecuteMsg) -> S

match msg {
ExecuteMsg::Swap { destination } => {
let amount = amount_from_funds(&info.funds, state.denom.clone())?;
let amount = amount_from_funds(&info.funds, &state.denom)?;
try_swap(deps, &env, &state, amount, destination)
}
ExecuteMsg::ReverseSwap {
Expand Down Expand Up @@ -481,7 +500,7 @@ fn try_deposit(deps: DepsMut, info: &MessageInfo, state: &State) -> StdResult<Re

let env_message_sender = &info.sender;

let amount = amount_from_funds(&info.funds, state.denom.clone())?;
let amount = amount_from_funds(&info.funds, &state.denom)?;
config(deps.storage).update(|mut state| -> StdResult<_> {
state.supply += amount;
Ok(state)
Expand Down Expand Up @@ -719,7 +738,7 @@ fn try_renounce_role(deps: DepsMut, info: &MessageInfo, role: String) -> StdResu
* ***************** Helpers *****************
* ***************************************************/

pub fn amount_from_funds(funds: &[Coin], denom: String) -> StdResult<Uint128> {
pub fn amount_from_funds(funds: &[Coin], denom: &str) -> StdResult<Uint128> {
for coin in funds {
if coin.denom == denom {
return Ok(coin.amount);
Expand Down
2 changes: 2 additions & 0 deletions fetch/cosmwasm_contract/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ pub const ERR_EON: &str = "[FET_ERR_EON] Tx doesn't belong to current relayEon";
// refund
pub const ERR_INVALID_SWAP_ID: &str = "[FET_ERR_INVALID_SWAP_ID] Invalid swap id";
pub const ERR_ALREADY_REFUNDED: &str = "[FET_ERR_ALREADY_REFUNDED] Refund was already processed";
pub const ERR_STATE_ERROR: &str = "[FET_ERR_STATE_ERR] Contract state not properly initialised";

#[derive(Error, Debug)]
pub enum ContractError {
#[error("{0}")]
Expand Down
1 change: 1 addition & 0 deletions fetch/cosmwasm_contract/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ pub mod error;
pub mod msg;
pub mod state;

pub mod migration;
#[cfg(test)]
pub mod tests;
29 changes: 29 additions & 0 deletions fetch/cosmwasm_contract/src/migration.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
use crate::access_control::wipe_all_roles_from_storage;
use crate::contract::initialise_contract_state;
use crate::error::ERR_ALREADY_REFUNDED;
use crate::msg::MigrateMsg;
use crate::state::is_state_valid;
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

// Remove all previously registered roles
wipe_all_roles_from_storage(deps.storage);

// Ensure correct contract state
if let Some(re_init) = msg.re_init {
initialise_contract_state(
deps.storage,
&env,
&re_init.admin,
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

&re_init.init_msg,
)?;
} else if !is_state_valid(deps.storage) {
return Err(StdError::generic_err(ERR_ALREADY_REFUNDED));
}

Ok(Response::default())
}
21 changes: 21 additions & 0 deletions fetch/cosmwasm_contract/src/msg.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};

use crate::contract::DEFAULT_DENOM;
use cosmwasm_std::Addr;

//use crate::cosmwasm_bignumber::{Uint256};
Expand All @@ -20,6 +21,12 @@ pub struct InstantiateMsg {
pub denom: Option<String>,
}

impl InstantiateMsg {
pub fn get_denom(&self) -> &str {
self.denom.as_deref().unwrap_or(DEFAULT_DENOM)
}
}

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
#[serde(rename_all = "snake_case")]
pub enum ExecuteMsg {
Expand Down Expand Up @@ -155,3 +162,17 @@ pub struct PausedSinceBlockResponse {
pub struct DenomResponse {
pub denom: String,
}

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
pub struct ReInitMsg {
pub init_msg: InstantiateMsg,
pub admin: Addr,
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.

}

#[derive(Serialize, Deserialize, JsonSchema)]
pub struct MigrateMsg {
pub re_init: Option<ReInitMsg>,
}
9 changes: 5 additions & 4 deletions fetch/cosmwasm_contract/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use schemars::JsonSchema;
use serde::{Deserialize, Serialize};

use crate::msg::Uint128;
use cosmwasm_std::{Addr, Storage};
use cosmwasm_std::Storage;
use cosmwasm_storage::{
singleton, singleton_read, PrefixedStorage, ReadonlyPrefixedStorage, ReadonlySingleton,
Singleton,
Expand All @@ -28,9 +28,6 @@ pub struct State {
pub paused_since_block_public_api: u64,
pub paused_since_block_relayer_api: u64,
pub denom: String,

// optimization FIXME(LR) Not needed any more with version 0.10.0
pub contract_addr_human: Addr,
}

pub fn config(storage: &mut dyn Storage) -> Singleton<State> {
Expand All @@ -50,3 +47,7 @@ pub fn refunds_have(swap_id: u64, storage: &dyn Storage) -> bool {
let store = ReadonlyPrefixedStorage::new(storage, REFUNDS_KEY);
store.get(&swap_id.to_be_bytes()).is_some()
}

pub fn is_state_valid(storage: &dyn Storage) -> bool {
config_read(storage).load().is_ok()
}
Loading
Loading