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

Replace timestamp with block number #866

Merged
merged 8 commits into from
May 31, 2024

Conversation

JesseAbram
Copy link
Member

Closes #857

@JesseAbram JesseAbram marked this pull request as ready for review May 30, 2024 23:04
@JesseAbram JesseAbram requested review from ameba23 and HCastano May 30, 2024 23:04
@frankiebee
Copy link
Contributor

💛

Copy link
Contributor

@ameba23 ameba23 left a comment

Choose a reason for hiding this comment

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

🥇

use std::time::{Duration, UNIX_EPOCH};
UNIX_EPOCH + Duration::from_secs(js_sys::Date::now() as u64)
}
#[cfg(not(feature = "full-client-wasm"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good that we no longer need to worry about the system clock on different platforms here

@@ -48,7 +48,7 @@ use crate::{
#[derive(Debug, Deserialize, Serialize, Clone)]
pub struct Keys {
pub keys: Vec<String>,
pub timestamp: SystemTime,
pub block_number: BlockNumber,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah so we also change this in the sync_kvdb payload

Copy link
Member Author

Choose a reason for hiding this comment

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

ya cuz it shares the check_stale function but I wouldn't worry to much, gonna need a whole re write when we go to threshold

Copy link
Collaborator

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

👍

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 40 to 44
let block_difference = if chain_block_number > user_block_number {
chain_block_number.checked_sub(user_block_number).ok_or(ValidationErr::StaleMessage)?
} else {
user_block_number.checked_sub(chain_block_number).ok_or(ValidationErr::StaleMessage)?
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use u32::abs_diff here to simplify this logic

@@ -206,7 +208,7 @@ async fn test_sync_kvdb() {
assert_eq!(result_4.status(), 500);
assert_eq!(result_4.text().await.unwrap(), "Forbidden Key");

keys.timestamp = keys.timestamp.checked_sub(TIME_BUFFER).unwrap();
keys.block_number = keys.block_number + BLOCK_BUFFER + 10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we add an extra 10 here if we already have the BLOCK_BUFFER?

Copy link
Member Author

Choose a reason for hiding this comment

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

I need a plus 1 to put it over the buffer, and idk I just made it 10

@@ -235,6 +242,7 @@ pub async fn get_and_store_values(
recip_server_info: ServerInfo<subxt::utils::AccountId32>,
signer: &PairSigner<EntropyConfig, sr25519::Pair>,
x25519_secret: &StaticSecret,
rpc: &LegacyRpcMethods<EntropyConfig>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be nice to not need to pass this in, but 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

ya but wouldn't worry about it, all this is gonna need a re write when we go to threshold

JesseAbram and others added 3 commits May 31, 2024 09:11
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
@JesseAbram JesseAbram merged commit e625559 into master May 31, 2024
13 checks passed
@JesseAbram JesseAbram deleted the replace-timestamp-with-blocknumber branch May 31, 2024 14:25
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.

Timestamps coming from js are too unreliable, switch to block number instead
4 participants