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

✨ Add CurrencyAccount to handle all Currency related logic #3779

Merged

Conversation

greymistcube
Copy link
Contributor

@greymistcube greymistcube commented May 7, 2024

The implementation is pretty convoluted at the moment. 🙄
There are numerous issues regarding Currency:

  • It isn't at all clear who should be responsible of keeping a state "consistent" regarding a Currency.
    • "Consistency" is purely enforced by code, not stored data.
    • The whole Currency manipulation pipeline is running on top of the assumption that a state is "consistent"
    • There are two relations that can be checked purely from state + Currency:
      • Whether TotalSupply does not exceed MaximumSupply
      • Whether TotalSupply is being tracked and is correct.
  • TotalSupplyTrackable loses its meaning starting with BlockMetadata.CurrencyAccountProtocolVersion.
  • Read operation throwing an Exception for TotalSupply.
    • It is put in as a safety measure, but in reality, this does not achieve anything.
    • Handling of said Exception regarding TotalSupply must be handled from IAction side (or any outside service), and its interpretation requires additional knowledge about Currency and the behavior of read operation of TotalSupply.
    • Besides, it has never been used.
  • Mint/burn authority validation has nothing to do with the underlying state.
    • One solution would be to enforce business logic through code analysis where a call to MintAsset() or BurnAsset() should be preceded by authority validation.
    • Falling short of that, the responsibility falls to the developers of IActions.

This is partially due to information about Currency not being recorded in a state and migration to CurrencyAccount model
does not permit adding in this missing information.

I'll be making some Currency spec and API changes in separate PRs to mitigate some of these issues. 😶

@greymistcube greymistcube force-pushed the feature/currency-account branch 2 times, most recently from 8aecb53 to 3fd7917 Compare May 8, 2024 06:56
@greymistcube greymistcube changed the title Feature/currency account ✨ Add CurrencyAccount to handle all Currency related logic May 8, 2024
@greymistcube greymistcube self-assigned this May 9, 2024
@greymistcube greymistcube marked this pull request as ready for review May 9, 2024 01:30
OnedgeLee

This comment was marked as outdated.

@greymistcube
Copy link
Contributor Author

greymistcube commented May 9, 2024

@OnedgeLee

Do this PR contains state migration test? Or, existing test can cover currency migration?

Migration is mostly covered by ActionEvaluatorTest.Migration.cs. Although now that I look at it, I should add Currency manipulation to the action used in MigrationThroughBlock test. 😗

@greymistcube
Copy link
Contributor Author

@OnedgeLee

I've added an additional test regarding migration. See 4e5fc2f.

@OnedgeLee OnedgeLee self-requested a review May 9, 2024 07:55
@greymistcube greymistcube merged commit e3f0ce5 into planetarium:main May 9, 2024
22 checks passed
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.

3 participants