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(backend): set user token #1554

Merged
merged 23 commits into from
Jun 26, 2024
Merged

Conversation

peterpeterparker
Copy link
Member

@peterpeterparker peterpeterparker commented Jun 25, 2024

Motivation

Given that we are required to extend the dApp to allow users to enable and disable ERC20 tokens as well, we decided to take a simplistic approach - i.e., extending the current entities with a similar mechanism as the one we implemented for CustomToken by adding an "enabled" flag to UserToken. We discussed using the "custom token" types, but extending the stable tree map we already have allows us to avoid a migration at this time that does not necessarily add value for end users. This new approach also means that user tokens (ERC20) won't be added and deleted anymore in the state, which is why this PR deprecates the related function and adds a setter, again similar to the custom token approach.

Changes

  • deprecate add_user_token and remove_user_token
  • add set_user_token and set_many_user_tokens for Erc20 tokens. Pattern similar to "custom token" which we added recently and are using for Icrc on the IC
  • add field enabled: Option<bool> in UserToken type
  • assert that the field is set when tokens are provided
  • generate did and js types
  • add a TODO in frontend code already resolved with all the waiting draft PRs

Notes

In another PR soon, I'll remove the deprecated functions.

Tests

Provide related tests and upgrade as well.

@peterpeterparker peterpeterparker marked this pull request as ready for review June 25, 2024 17:30
@peterpeterparker peterpeterparker requested a review from a team as a code owner June 25, 2024 17:30
@peterpeterparker
Copy link
Member Author

This PR blocks several other PRs (#1557, #1566, #1570, #1572, #1573, #1574), which are all blocking each other and conflicting with the realization of #1568 and #1578.

As a result, it has become too cumbersome to develop the expected features based on the draft work. That's why I am merging this PR.

Please, @bitdivine, review it as it was not merged yet, and let's improve or correct what's needed in separate PRs afterward.

@peterpeterparker peterpeterparker merged commit d114724 into main Jun 26, 2024
14 checks passed
@peterpeterparker peterpeterparker deleted the feat/set-user-token-enabled branch June 26, 2024 06:55
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. There are a couple of small tweaks that could be made but that is all.

src/backend/src/lib.rs Show resolved Hide resolved
src/backend/tests/it/upgrade/token_enabled.rs Show resolved Hide resolved
src/backend/tests/it/user_token.rs Show resolved Hide resolved
src/backend/tests/it/user_token.rs Show resolved Hide resolved
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