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

feat: token version #1137

Merged
merged 21 commits into from
Apr 18, 2024
Merged

feat: token version #1137

merged 21 commits into from
Apr 18, 2024

Conversation

peterpeterparker
Copy link
Member

@peterpeterparker peterpeterparker commented Apr 17, 2024

Motivation

Because the UI/UX of custom tokens revolves around enabling and disabling tokens, I think it is worth adding a timestamp (UPDATE: replaced by a version counter following review) to the information to prevent use cases where the user operates on multiple devices simultaneously and would overwrite changes made on one device with those from another.

It may be a not-too-frequent scenario but, given that we are developing this new feature which is not yet live, I think it is the right time to add these kinds of assertions.

That's why this PR adds a version: Option<u64> to the existing UserTokens and CustomTokens.

Notes

In this PR, we are adding new information within a struct of stable memory that already exists on mainnet but, given that we introduce it as an option, it should be backward compatible. There is also a test about it.

Given that the backend sets the version when the entity is saved, it would be handy to return the new entity when setter and add functions are called. However, the Oisy frontend does not currently require it as we are reloading all the user tokens after changes. Therefore, this PR does not change those functions' returns.

For backwards compatibility, I did not modified the existing remove_from_user_token function which accept an ID.

Did files will be updated in a separate PR that also adapts the frontend code.

@peterpeterparker peterpeterparker marked this pull request as ready for review April 17, 2024 14:32
src/backend/tests/it/upgrade.rs Show resolved Hide resolved
src/backend/tests/it/upgrade.rs Outdated Show resolved Hide resolved
src/backend/tests/it/upgrade.rs Outdated Show resolved Hide resolved
src/backend/tests/it/upgrade.rs Outdated Show resolved Hide resolved
src/backend/tests/it/utils/assertion.rs Outdated Show resolved Hide resolved
src/backend/tests/it/upgrade.rs Outdated Show resolved Hide resolved
@@ -1,5 +1,6 @@
use shared::types::custom_token::CustomToken;
use shared::types::token::UserToken;
use shared::types::TokenTimestamp;

Copy link
Member

Choose a reason for hiding this comment

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

Suggestion:

If you add these traits to UserToken:

    #[derive(CandidType, Deserialize, Clone, Debug, Eq, PartialEq)]
    pub struct UserToken {

you can do assert_eq!(&one_token_list, &another_token_list, "My lists aren't the same!") without needing a dedicated comparison function.

If the default definition of Eq does not suit, it is possible to define a custom one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure we can do that in another PR. Original did not wanted to change the existing struct but, we are passed that now ;)

src/backend/tests/it/utils/assertion.rs Outdated Show resolved Hide resolved
src/backend/tests/it/utils/assertion.rs Outdated Show resolved Hide resolved
src/backend/tests/it/utils/assertion.rs Outdated Show resolved Hide resolved
@peterpeterparker
Copy link
Member Author

Printing out the token in the assertions of the test requires implementing display. I don't think that's part of this PR, so I've reverted printing out and we can do that in another PR.

error[E0277]: `T` doesn't implement `std::fmt::Display`
  --> src/backend/tests/it/utils/assertion.rs:33:80
   |
33 |         assert!(token.get_timestamp().is_some(), "Token has no timestamp: {}", token);
   |                                                                                ^^^^^ `T` cannot be formatted with the default formatter
   |
   = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
   = note: this error originates in the macro `$crate::const_format_args` which comes from the expansion of the macro `assert` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider further restricting this bound
   |
30 |     T: TokenTimestamp + std::fmt::Display,
   |                       +++++++++++++++++++

@peterpeterparker peterpeterparker changed the title feat: token timestamp feat: token version Apr 18, 2024
Copy link
Member

@bitdivine bitdivine left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@peterpeterparker peterpeterparker merged commit 0bdbf0b into main Apr 18, 2024
6 checks passed
@peterpeterparker peterpeterparker deleted the feat/backend-timestamp branch April 18, 2024 14:36
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