feat: keep track of user custom tokens #1100
Conversation
| @@ -51,10 +52,9 @@ type Token = record { | |||
| symbol : opt text; | |||
| }; | |||
| type TokenId = record { chain_id : nat64; contract_address : text }; | |||
| type UserToken = variant { Icrc : IcrcToken }; | |||
| type UserToken = record { token : CustomToken; enabled : bool }; | |||
There was a problem hiding this comment.
While this is a breaking change, I understand that the code on the left has not been deployed yet so this is all new code. No migration needed. Ditto with breaking changes below.
TODOs to be done in other PRs:
- Add git commit to deployed metadata, so that it is easy to find out which code is in prod.
- Add downgrade/upgrade tests so that any real breaking changes are caught by CI.
There was a problem hiding this comment.
Add git commit to deployed metadata...
The version deployed in prod is the last release https://github.com/dfinity/oisy-wallet/releases/tag/v0.0.13 - i.e. commit 2d267cf
Add downgrade/upgrade tests...
There are already downgrade/upgrade tests. There is no particular test for this yet given that it does not exist on mainnet. After release we can develop such a new test (see upgrade.rs for reference)
| @@ -65,6 +65,8 @@ service : (Arg) -> { | |||
| personal_sign : (text) -> (text); | |||
| remove_user_custom_token : (UserTokenId) -> (); | |||
| remove_user_token : (TokenId) -> (); | |||
| set_many_user_custom_tokens : (vec UserToken) -> (); | |||
| set_user_custom_token : (UserToken) -> (); | |||
There was a problem hiding this comment.
Out of curiosity, why "user_custom" as opposed to "user" or "custom"? What concept should each of those terms conjure up in my mind? What variations or combinations are likely to exist in future?
Below we have both UserToken and CustomToken so it looks as if the difference is actually significant.
There was a problem hiding this comment.
Mostly because existing ERC20 endpoints are defined as add_user_token and remove_user_token and we discussed last week that in the future we migrate those to the new tokens feature we are introduced, therefore felt like using a naming pattern that was close - e.g. add_user_token -> ``add_user_custom_ token`.
Furthermore, the name Token is already used for the struct of the ERC20 token. So to spare a refactoring, I picked "custom" as prefix.
Not against renaming ERC20 related existing code and using other terms for the new code.
We need one struct that contains the token, enabled flag and timestamp and a struct that contains the effective token (ICRC or something else).
Any suggestions?
|
I am merging this PR as it is now blocking my progress in developing the feature, as discussed with @bitdivine. Given that it is not yet in production, we can, of course, iterate afterwards. Please, Max, let me know what you think, particularly about the naming question. |
Motivation
The design of the wallet (product/frontend) for ICRC tokens was ultimately designed with a slightly different twist than the design of the existing ERC20 support. The idea is to allow users to enable or disable tokens. This means that we can potentially resolve such a feature, "enabling disabling token," with the current design of the backend which allows adding and removing custom tokens, but we would not be able to show the user disabled custom tokens across sessions.
For example, let's say I sign in in Oisy and I set up (enable) a custom ICRC token such as Windoge98. A few weeks later, I sign in to Oisy and disable the token. Given the current implementation, this would imply removing it from the backend. I sign out and come back a few days later, sign in to Oisy, and then we cannot display Windoge98 as disabled because the token isn't in memory anymore.
For this reason, I propose that we add an "enabled" status to the custom token in the backend.
Furthermore, given that the user can potentially enable and disable multiple tokens at once, I also propose to add an additional endpoint to do so.
Remaining work
Given that the backend will support adding, updating, and removing, and the fact that the feature is not yet live, I think we should add a timestamp to the entities to ensure that only the most up-to-date entity can be modified. This will be particularly useful if a user works on multiple devices.
Changes
enabledflag toUserTokenstructset_many_user_custom_tokensto support editing multiple tokens at onceadd_user_custom_tokentoset_user_custom_tokengiven that the function can both add and updatefindfunction to compare always id given than the structUserTokencan differs if the flagenabledis updatedDesign
To better frame the motivation, here is a screenshot of the design: