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

[sui-framework] Adds Balance module #1573

Merged
merged 9 commits into from
Apr 27, 2022
Merged

[sui-framework] Adds Balance module #1573

merged 9 commits into from
Apr 27, 2022

Conversation

damirka
Copy link
Contributor

@damirka damirka commented Apr 25, 2022

Covers #1233.

This PR has a draft status and used for demonstration purposes.
UPD: converted to normal PR as it seems complete.

Pros

  1. Balance is a small and nice module with zero dependencies;
  2. It is possible to implement custom wrappers on a Balance type;
  3. Coin is transferable wrapper around Balance; possibly not needed in some applications;
  4. Separation of modules resolves name collisions and helps readability (Coin is for minting and handling accounts, balance is its internals);
  5. Balance doesn't use TxContext at all and has no IDs - which is great!

Cons

  1. Multi-layer solutions complicate things;
  2. We'll need to have a guide/tutorial on how to use them (def!); and when to use Coin or Balance;
  3. Since Coin handles minting, Balance is always bound to it and cannot be used freely for custom implementations of finances;
  4. To take a Coin from a balance, few calls needed:
let balance = Balance { value: 100 }; // roughly
let coin = Coin::from_balance(Balance::split(&mut balance, 50), ctx);

@damirka damirka added the move label Apr 25, 2022
@damirka damirka added this to the DevNet milestone Apr 25, 2022
@damirka damirka self-assigned this Apr 25, 2022
@damirka damirka requested a review from kchalkias April 25, 2022 12:06
@damirka damirka changed the title [WIP] Adds Balance module [sui-framework] Adds Balance module Apr 25, 2022
@damirka damirka marked this pull request as ready for review April 25, 2022 15:06
@tnowacki
Copy link
Contributor

A few random comments:

  1. "Since Coin handles minting, Balance is always bound to it and cannot be used freely for custom implementations of finances;". You could handle this with a minting capability. But then you will have like Balance<MintCap, CoinType> everywhere. Could be a nice usecase for type aliases though (Coin::Balance<CoinType> = Balance<Coin::MintCap, CoinType>)
  2. For me it feels a bit strange to have Coin be the one with an ID, but maybe that is too much time in Diem... I could imagine something like (waves hands) Coin { value: u64 } and Wallet { id: VersionedId, balance: Coin } (not that the term "Wallet" is overused at all or anything)

}

/// Create an empty `Balance` for type `T`.
public fun empty<T>(): Balance<T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels a bit asymmetric with Coin::zero to me. I don't have a strong preference for empty vs zero, but I think we should probably pick the same name for both.

module Sui::Balance {
friend Sui::Coin;

const ENONZERO: u64 = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: comments on error codes and more descriptive names

@damirka damirka enabled auto-merge (squash) April 27, 2022 15:40
@damirka damirka merged commit d5a3882 into main Apr 27, 2022
@damirka damirka deleted the ds/coin-balance branch April 27, 2022 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants