Skip to content

Commit

Permalink
refactor: remove rc wrapper
Browse files Browse the repository at this point in the history
  • Loading branch information
gluax committed Aug 22, 2024
1 parent bdbc388 commit 0cce8e3
Show file tree
Hide file tree
Showing 13 changed files with 86 additions and 128 deletions.
2 changes: 1 addition & 1 deletion contract/src/msgs/data_requests/execute/commit_result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ impl ExecuteHandler for execute::commit_result::Execute {
// add the commitment to the data request
let commitment = Hash::from_hex_str(&self.commitment)?;
dr.commits.insert(self.public_key.clone(), commitment);
state::commit(deps.storage, dr_id.into(), dr)?;
state::commit(deps.storage, dr_id, dr)?;

Ok(Response::new().add_attribute("action", "commit_data_result").add_event(
Event::new("seda-commitment").add_attributes([
Expand Down
2 changes: 1 addition & 1 deletion contract/src/msgs/data_requests/execute/post_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ impl ExecuteHandler for execute::post_request::Execute {

height: env.block.height,
};
state::post_request(deps.storage, dr_id.into(), dr)?;
state::post_request(deps.storage, dr_id, dr)?;

Ok(res)
}
Expand Down
4 changes: 1 addition & 3 deletions contract/src/msgs/data_requests/execute/reveal_result.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use std::rc::Rc;

use super::*;
use crate::state::CHAIN_ID;

Expand All @@ -8,7 +6,7 @@ impl ExecuteHandler for execute::reveal_result::Execute {
/// This removes the data request from the pool and creates a new entry in the data results.
fn execute(self, deps: DepsMut, env: Env, _info: MessageInfo) -> Result<Response, ContractError> {
// find the data request from the committed pool (if it exists, otherwise error)
let dr_id: Rc<Hash> = Hash::from_hex_str(&self.dr_id)?.into();
let dr_id = Hash::from_hex_str(&self.dr_id)?;
let mut dr = state::load_request(deps.storage, &dr_id)?;

// error if reveal phase for this DR has not started (i.e. replication factor is not met)
Expand Down
26 changes: 12 additions & 14 deletions contract/src/msgs/data_requests/state/data_requests_map.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use std::rc::Rc;

use super::*;

pub struct DataRequestsMap<'a> {
Expand All @@ -21,7 +19,7 @@ impl DataRequestsMap<'_> {
self.reqs.has(store, key)
}

fn add_to_status(&self, store: &mut dyn Storage, key: Rc<Hash>, status: &DataRequestStatus) -> StdResult<()> {
fn add_to_status(&self, store: &mut dyn Storage, key: Hash, status: &DataRequestStatus) -> StdResult<()> {
match status {
DataRequestStatus::Committing => self.committing.add(store, key)?,
DataRequestStatus::Revealing => self.revealing.add(store, key)?,
Expand All @@ -31,7 +29,7 @@ impl DataRequestsMap<'_> {
Ok(())
}

fn remove_from_status(&self, store: &mut dyn Storage, key: Rc<Hash>, status: &DataRequestStatus) -> StdResult<()> {
fn remove_from_status(&self, store: &mut dyn Storage, key: Hash, status: &DataRequestStatus) -> StdResult<()> {
match status {
DataRequestStatus::Committing => self.committing.remove(store, key)?,
DataRequestStatus::Revealing => self.revealing.remove(store, key)?,
Expand All @@ -44,7 +42,7 @@ impl DataRequestsMap<'_> {
pub fn insert(
&self,
store: &mut dyn Storage,
key: Rc<Hash>,
key: Hash,
req: DataRequest,
status: &DataRequestStatus,
) -> StdResult<()> {
Expand All @@ -58,12 +56,12 @@ impl DataRequestsMap<'_> {
Ok(())
}

fn find_status(&self, store: &dyn Storage, key: Rc<Hash>) -> StdResult<DataRequestStatus> {
if self.committing.has(store, key.clone()) {
fn find_status(&self, store: &dyn Storage, key: Hash) -> StdResult<DataRequestStatus> {
if self.committing.has(store, key) {
return Ok(DataRequestStatus::Committing);
}

if self.revealing.has(store, key.clone()) {
if self.revealing.has(store, key) {
return Ok(DataRequestStatus::Revealing);
}

Expand All @@ -77,7 +75,7 @@ impl DataRequestsMap<'_> {
pub fn update(
&self,
store: &mut dyn Storage,
key: Rc<Hash>,
key: Hash,
dr: DataRequest,
status: Option<DataRequestStatus>,
) -> StdResult<()> {
Expand All @@ -89,7 +87,7 @@ impl DataRequestsMap<'_> {
// If we need to update the status, we need to remove the key from the current status
if let Some(status) = status {
// Grab the current status.
let current_status = self.find_status(store, key.clone())?;
let current_status = self.find_status(store, key)?;
// world view = we should only update from committing -> revealing -> tallying.
// Either the concept is fundamentally flawed or the implementation is wrong.
match current_status {
Expand Down Expand Up @@ -117,8 +115,8 @@ impl DataRequestsMap<'_> {
}

// remove from current status, then add to new one.
self.remove_from_status(store, key.clone(), &current_status)?;
self.add_to_status(store, key.clone(), &status)?;
self.remove_from_status(store, key, &current_status)?;
self.add_to_status(store, key, &status)?;
}

// always update the request
Expand All @@ -137,14 +135,14 @@ impl DataRequestsMap<'_> {
/// Removes an req from the map by key.
/// Swaps the last req with the req to remove.
/// Then pops the last req.
pub fn remove(&self, store: &mut dyn Storage, key: Rc<Hash>) -> Result<(), StdError> {
pub fn remove(&self, store: &mut dyn Storage, key: Hash) -> Result<(), StdError> {
if !self.has(store, &key) {
return Err(StdError::generic_err("Key does not exist"));
}

// world view = we only remove a data request that is done tallying.
// Either the concept is fundamentally flawed or the implementation is wrong.
let current_status = self.find_status(store, key.clone())?;
let current_status = self.find_status(store, key)?;
assert_eq!(
current_status,
DataRequestStatus::Tallying,
Expand Down
101 changes: 48 additions & 53 deletions contract/src/msgs/data_requests/state/data_requests_map_tests.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use std::rc::Rc;

use test::test_helpers::{calculate_dr_id_and_args, construct_dr};
use testing::MockStorage;

Expand Down Expand Up @@ -30,16 +28,13 @@ impl TestInfo<'_> {
}

#[track_caller]
fn assert_status_key_to_index(&self, status: &DataRequestStatus, key: Rc<Hash>, index: Option<u32>) {
fn assert_status_key_to_index(&self, status: &DataRequestStatus, key: Hash, index: Option<u32>) {
let status_map = match status {
DataRequestStatus::Committing => &self.map.committing,
DataRequestStatus::Revealing => &self.map.revealing,
DataRequestStatus::Tallying => &self.map.tallying,
};
assert_eq!(
index,
status_map.key_to_index.may_load(&self.store, key.into()).unwrap()
);
assert_eq!(index, status_map.key_to_index.may_load(&self.store, key).unwrap());
}

#[track_caller]
Expand All @@ -56,26 +51,26 @@ impl TestInfo<'_> {
}

#[track_caller]
fn insert(&mut self, key: Rc<Hash>, value: DataRequest) {
fn insert(&mut self, key: Hash, value: DataRequest) {
self.map
.insert(&mut self.store, key, value, &DataRequestStatus::Committing)
.unwrap();
}

#[track_caller]
fn insert_removable(&mut self, key: Rc<Hash>, value: DataRequest) {
fn insert_removable(&mut self, key: Hash, value: DataRequest) {
self.map
.insert(&mut self.store, key, value, &DataRequestStatus::Tallying)
.unwrap();
}

#[track_caller]
fn update(&mut self, key: Rc<Hash>, dr: DataRequest, status: Option<DataRequestStatus>) {
fn update(&mut self, key: Hash, dr: DataRequest, status: Option<DataRequestStatus>) {
self.map.update(&mut self.store, key, dr, status).unwrap();
}

#[track_caller]
fn remove(&mut self, key: Rc<Hash>) {
fn remove(&mut self, key: Hash) {
self.map.remove(&mut self.store, key).unwrap();
}

Expand All @@ -97,12 +92,12 @@ impl TestInfo<'_> {
}
}

fn create_test_dr(nonce: u128) -> (Rc<Hash>, DataRequest) {
fn create_test_dr(nonce: u128) -> (Hash, DataRequest) {
let args = calculate_dr_id_and_args(nonce, 2);
let id = nonce.to_string().hash();
let dr = construct_dr(args, vec![], nonce as u64);

(Rc::new(id), dr)
(id, dr)
}

#[test]
Expand All @@ -119,18 +114,18 @@ fn enum_map_insert() {
const TEST_STATUS: &DataRequestStatus = &DataRequestStatus::Committing;

let (key, val) = create_test_dr(1);
test_info.insert(key.clone(), val);
test_info.insert(key, val);
test_info.assert_status_len(1, TEST_STATUS);
test_info.assert_status_key_to_index(TEST_STATUS, key.clone(), Some(0));
test_info.assert_status_index_to_key(TEST_STATUS, 0, Some(*key));
test_info.assert_status_key_to_index(TEST_STATUS, key, Some(0));
test_info.assert_status_index_to_key(TEST_STATUS, 0, Some(key));
}

#[test]
fn enum_map_get() {
let mut test_info = TestInfo::init();

let (key, req) = create_test_dr(1);
test_info.insert(key.clone(), req.clone());
test_info.insert(key, req.clone());
test_info.assert_request(&key, Some(req))
}

Expand All @@ -141,7 +136,7 @@ fn enum_map_get_non_existing() {

test_info.assert_request(&"1".hash(), None);
test_info.assert_status_len(0, TEST_STATUS);
test_info.assert_status_key_to_index(TEST_STATUS, Rc::new("1".hash()), None);
test_info.assert_status_key_to_index(TEST_STATUS, "1".hash(), None);
test_info.assert_status_index_to_key(TEST_STATUS, 0, None);
}

Expand All @@ -150,7 +145,7 @@ fn enum_map_get_non_existing() {
fn enum_map_insert_duplicate() {
let mut test_info = TestInfo::init();
let (key, req) = create_test_dr(1);
test_info.insert(key.clone(), req.clone());
test_info.insert(key, req.clone());
test_info.insert(key, req);
}

Expand All @@ -162,15 +157,15 @@ fn enum_map_update() {
let (key1, dr1) = create_test_dr(1);
let (_, dr2) = create_test_dr(2);

test_info.insert(key1.clone(), dr1.clone());
test_info.insert(key1, dr1.clone());
test_info.assert_status_len(1, TEST_STATUS);
test_info.assert_status_key_to_index(TEST_STATUS, key1.clone(), Some(0));
test_info.assert_status_index_to_key(TEST_STATUS, 0, Some(*key1));
test_info.assert_status_key_to_index(TEST_STATUS, key1, Some(0));
test_info.assert_status_index_to_key(TEST_STATUS, 0, Some(key1));

test_info.update(key1.clone(), dr2.clone(), None);
test_info.update(key1, dr2.clone(), None);
test_info.assert_status_len(1, TEST_STATUS);
test_info.assert_status_key_to_index(TEST_STATUS, key1.clone(), Some(0));
test_info.assert_status_index_to_key(TEST_STATUS, 0, Some(*key1));
test_info.assert_status_key_to_index(TEST_STATUS, key1, Some(0));
test_info.assert_status_index_to_key(TEST_STATUS, 0, Some(key1));
}

#[test]
Expand All @@ -190,14 +185,14 @@ fn enum_map_remove_first() {
let (key2, req2) = create_test_dr(2);
let (key3, req3) = create_test_dr(3);

test_info.insert_removable(key1.clone(), req1.clone()); // 0
test_info.insert_removable(key2.clone(), req2.clone()); // 1
test_info.insert_removable(key3.clone(), req3.clone()); // 2
test_info.insert_removable(key1, req1.clone()); // 0
test_info.insert_removable(key2, req2.clone()); // 1
test_info.insert_removable(key3, req3.clone()); // 2

test_info.remove(key1.clone());
test_info.remove(key1);
test_info.assert_status_len(2, TEST_STATUS);
test_info.assert_status_key_to_index(TEST_STATUS, key3.clone(), Some(0));
test_info.assert_status_index_to_key(TEST_STATUS, 0, Some(*key3));
test_info.assert_status_key_to_index(TEST_STATUS, key3, Some(0));
test_info.assert_status_index_to_key(TEST_STATUS, 0, Some(key3));

// test that we can still get the other keys
test_info.assert_request(&key2, Some(req2.clone()));
Expand All @@ -217,12 +212,12 @@ fn enum_map_remove_last() {
let (key2, req2) = create_test_dr(2);
let (key3, req3) = create_test_dr(3);

test_info.insert_removable(key1.clone(), req1.clone()); // 0
test_info.insert_removable(key2.clone(), req2.clone()); // 1
test_info.insert_removable(key3.clone(), req3.clone()); // 2
test_info.insert_removable(key1, req1.clone()); // 0
test_info.insert_removable(key2, req2.clone()); // 1
test_info.insert_removable(key3, req3.clone()); // 2
test_info.assert_status_len(3, TEST_STATUS);

test_info.remove(key3.clone());
test_info.remove(key3);
test_info.assert_status_len(2, TEST_STATUS);
test_info.assert_status_index_to_key(TEST_STATUS, 2, None);
test_info.assert_status_key_to_index(TEST_STATUS, key3, None);
Expand All @@ -232,8 +227,8 @@ fn enum_map_remove_last() {
assert_eq!(test_info.get(&key2), Some(req2.clone()));

// test that the status indexes are still there
test_info.assert_status_index_to_key(TEST_STATUS, 0, Some(*key1));
test_info.assert_status_index_to_key(TEST_STATUS, 1, Some(*key2));
test_info.assert_status_index_to_key(TEST_STATUS, 0, Some(key1));
test_info.assert_status_index_to_key(TEST_STATUS, 1, Some(key2));
}

#[test]
Expand All @@ -246,13 +241,13 @@ fn enum_map_remove() {
let (key3, req3) = create_test_dr(3);
let (key4, req4) = create_test_dr(4);

test_info.insert_removable(key1.clone(), req1.clone()); // 0
test_info.insert_removable(key2.clone(), req2.clone()); // 1
test_info.insert_removable(key3.clone(), req3.clone()); // 2
test_info.insert_removable(key4.clone(), req4.clone()); // 3
test_info.insert_removable(key1, req1.clone()); // 0
test_info.insert_removable(key2, req2.clone()); // 1
test_info.insert_removable(key3, req3.clone()); // 2
test_info.insert_removable(key4, req4.clone()); // 3
test_info.assert_status_len(4, &DataRequestStatus::Tallying);

test_info.remove(key2.clone());
test_info.remove(key2);

// test that the key is removed
test_info.assert_status_len(3, &DataRequestStatus::Tallying);
Expand All @@ -264,23 +259,23 @@ fn enum_map_remove() {
test_info.assert_request(&key4, Some(req4));

// check that the status is updated
test_info.assert_status_key_to_index(TEST_STATUS, key1.clone(), Some(0));
test_info.assert_status_key_to_index(TEST_STATUS, key1, Some(0));
test_info.assert_status_key_to_index(TEST_STATUS, key2, None);
test_info.assert_status_key_to_index(TEST_STATUS, key3.clone(), Some(2));
test_info.assert_status_key_to_index(TEST_STATUS, key4.clone(), Some(1));
test_info.assert_status_key_to_index(TEST_STATUS, key3, Some(2));
test_info.assert_status_key_to_index(TEST_STATUS, key4, Some(1));

// check the status indexes
test_info.assert_status_index_to_key(TEST_STATUS, 0, Some(*key1));
test_info.assert_status_index_to_key(TEST_STATUS, 1, Some(*key4));
test_info.assert_status_index_to_key(TEST_STATUS, 2, Some(*key3));
test_info.assert_status_index_to_key(TEST_STATUS, 0, Some(key1));
test_info.assert_status_index_to_key(TEST_STATUS, 1, Some(key4));
test_info.assert_status_index_to_key(TEST_STATUS, 2, Some(key3));
test_info.assert_status_index_to_key(TEST_STATUS, 3, None);
}

#[test]
#[should_panic(expected = "Key does not exist")]
fn enum_map_remove_non_existing() {
let mut test_info = TestInfo::init();
test_info.remove(Rc::new(2.to_string().hash()));
test_info.remove(2.to_string().hash());
}

#[test]
Expand All @@ -291,7 +286,7 @@ fn get_requests_by_status() {
test_info.insert(key1, req1.clone());

let (key2, req2) = create_test_dr(2);
test_info.insert(key2.clone(), req2.clone());
test_info.insert(key2, req2.clone());
test_info.update(key2, req2.clone(), Some(DataRequestStatus::Revealing));

let committing = test_info.get_requests_by_status(DataRequestStatus::Committing, 0, 10);
Expand Down Expand Up @@ -336,7 +331,7 @@ fn get_requests_by_status_pagination() {
#[should_panic(expected = "Key does not exist")]
fn remove_from_empty() {
let mut test_info = TestInfo::init();
test_info.remove(Rc::new(1.to_string().hash()));
test_info.remove(1.to_string().hash());
}

#[test]
Expand All @@ -345,8 +340,8 @@ fn remove_only_item() {
const TEST_STATUS: &DataRequestStatus = &DataRequestStatus::Tallying;

let (key, req) = create_test_dr(1);
test_info.insert_removable(key.clone(), req.clone());
test_info.remove(key.clone());
test_info.insert_removable(key, req.clone());
test_info.remove(key);

test_info.assert_status_len(0, &DataRequestStatus::Tallying);
test_info.assert_status_index_to_key(TEST_STATUS, 0, None);
Expand Down
Loading

0 comments on commit 0cce8e3

Please sign in to comment.