-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fungible Tokens (spike) #1
Conversation
I think in general, this impl is too much of an implementation of a token rather than a library that would let other's build their own tokens. Not sure what the right balance should be in terms of inversion of control. |
I'm not sure in general if my use of @anorth do you see filecoin-project/FIPs#379 as a necessary building block for standard tokens? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great progress, we should merge it and keep iterating. And perhaps start using the issue tracker to track the many emerging items that we can't address all at once in code review.
One consideration for improving the API is efficiency of state writes for bulk operations. The current API will force some unnecessary state.save()
calls for intermediate states. E.g. intialising and then minting to a bunch of accounts up front, or making multiple transfers (from some non-standard token method). This API is pretty good for the basic cases and we want to keep them easy. We should think about how/whether to change this API to aid batching, or add an alternative style for less common but more complex calls. Maybe a closure that executes over state object, or a builder-style API that can abstract over it while batching.
The same then applies to the state API, which need not be so user-friendly (our lib is the main user) but wants to avoid serialising state objects unnecessarily. A closure pattern could ork here. There's some amount of necessary serialisation internal to HAMT and similar structures, though.
|
||
{ | ||
// TODO: helper functions for saving hamts?, this can probably be done more efficiently rather than | ||
// getting the root allowance map again, by abstracting the nested hamt structure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks close to what I expect, except for loading the root allowances map again. You should just load it explicitly at the top because we'll need to update it.
|
||
/// Increase the allowance between owner and spender by the specified value | ||
/// | ||
/// Caller must ensure that value is non-negative. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here again how about a single change_allowance
function
if let Some(mut map) = allowance_map { | ||
map.delete(&spender)?; | ||
if map.is_empty() { | ||
let mut root_allowance_map = self.get_allowances_map(bs)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above, just get this root map explicitly at the start rather than hiding it in get_owner_allowance_map
) | ||
})?; | ||
|
||
if new_allowance.lt(&TokenAmount::zero()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_negative
/// Burn tokens from the caller's account, decreasing the total supply | ||
/// | ||
/// When burning tokens: | ||
/// - Any holder MUST be allowed to burn their own tokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these rules are probably not strictly necessary for some interesting tokens
I think it's fine.
No, these are independent. |
No description provided.