This exposes an interesting chicken and egg problem.

In the proposal, I wrote:

An uninitialised pubkey address can receive tokens (because receiver hook will initialise it), but cannot be authorized as a spender

I was about to say you won't always have the actor ID so change to address, but I realise that we can't have the token state reflecting the new balances persisted if we don't know the receiving actor ID.

I think the required workaround will be that address resolution (in the actor code) needs to initialise actors if necessary. See resolve_to_id_addr in the builtin-actors runtime/src/builtin/shared.rs. And then this API is ok. (Defer this for a follow-up)

Originally posted by @anorth in #28 (comment)

A simple example actor showing the bare minimum to get a compliant token running. It should have a public mint function that anyone can use to increase their own balance.

Follow-up from issue #88 and PR #93 (comment)

The receiver hook itself isn't limited to FRC46 tokens anymore and should find use with NFTs and other assets. It's worth pulling that out into a separate crate so it's easier to share around and use without pulling in all the token code.

This issue comes from a review by @Kubuxu.

Token::set_balance is the only place where the Token's abstraction leaks the need for total_supply accounting, and no utilities are provided to resolve that.

Suggestion: either turn set_balance private or make it do total_supply accounting or provide utilities to fix total_supply.

This was added in #98 alongside set_allowance, which is much easier to use properly.

Because rust doesn't let us derive builtin traits on external types this would make development a bit nicer. There's a need for message params (see here) but probably helpful for all exported types.

While not a standard actor method, users may want to directly specify an allowance.

Along these lines, think about any other basic CRUD methods the library shoudl expose for dev convenience (while maintaining its internal invariants).

All standardised method numbers/invocations should be handled by a single line of library code.

Unhandled method numbers are delegated to the actor author so that they can extend functionality (e.g. for minting)

This should use the (to be written) proc macros for dispatch and calling

#3
#4

I don't know if it's a good idea for the library code to be doing this instead of handling state loading on the user implementation side (like we're doing for fungible tokens). It'll be ok for now since it's going into a dev branch but I think it'll cause problems in actor code that carries additional state of its own beyond the NFTState

It does open up one easy optimisation though: we can pass through the original MintReturn data in the intermediate struct and simply return that if there's no change once the hook call completes. I've been playing with the idea here (sending original data) and here (trying to centralise the 'has state changed' check) for the fungible token

Originally posted by @abright in #138 (comment)

Not much comments on Fungible Token Standards, do have a lot to say on Non Fungible Token Standards. But I do notice many new chains are using u128/u64 over BigInt, simply because they are primitives and more than sufficient to cover all use cases.

Originally posted by @Tom-OriginStorage in filecoin-project/FIPs#407 (comment)

Provide high level overview of the various crates/usage scenarios

The balance granularity is not stored in state, but supplied from outside. Check that all balances are whole multiples of the granularity.

Callers of the state invariant check will often want to check additional app-specific invariants, including reconciliation with the state of other actors.

Return a summary of the state from check_invariants. This doesn't need to be CBOR-serializable.

• TreeMap of all balances
• TreeMap of all allowances
• Total supply

Implement the change described in filecoin-project/FIPs#434

A testing function that examines the HAMTs, AMTs within the NFT state and checks that all keys, balances, etc. are valid and consistent

This should be called at the end (and any point during the execution) of all state unit tests

Currently, we have supposed a bitwidth of 5 is ideal for the HAMTs.

This constant should be determined empirically by simulating actors under use with large balances and transfer volume.

It should probably also be overridable by consumers if they wish to customise.

FIL Token Actor

This creates a FIP-??? compliant fungible token that wraps the value of of
native FIL.

Calling the mint (non-standard) method on this actor will increase the
caller's WFIL balance by the amount of FIL sent in the message. The WFIL can be
transferred between any FIP-??? compliant wallets (which have token_received
hook) by either direct transfer or delegated transfer.

https://etherscan.io/tx/0x96a7155b44b77c173e7c534ae1ceca536ba2ce534012ff844cf8c1737bc54921

Direct transfer flow

Call transfer(TransferParams) to directly transfer tokens from the caller's
wallet to a receiving address.

Delegated transfer flow

1. Call increase_allowance(AllowanceParams) to increase the "spenders"
   allowance
2. The "spender" can then call this actor with
   transfer_from(TransferFromParams) to

Transferring tokens to this actor

Transferring WFIL to the address of this actor itself will result in the WFIL
being burnt and the corresponding FIL being credited back. This prevents the
case where tokens are unintentionally locked in to contracts that are unable to
receive them. This flow requires the actor to implement its own token_received
hook.

However, also compliant with the token standard would be for this actor to omit
the token_received hook. In this case, transfers to the contract itself would
simply be rejected, which also prevents unintentional loss of tokens.

See: #133 (comment)

Add a transaction like capability to the state level of the NFT library so that multiple state updates can be batched and the blockstore flushes can be delayed.

Currently it only has a single integration test that mints tokens, this should be extended (or new tests added) to involve allowances, transfers and burn so all the major operations are covered. Need to be careful because those tests are quite heavy and could lead to even longer CI runs.

Currently when balances and allowances are updated, the entire map is read from the blockstore and then written back.

It may be more efficient to shard the balance map by ActorID so that the number of bytes written and read to/from the blockstore is lower. If sharding factor should be configurable if indeed it is overall more gas efficient.

If any map is changed, it will still be necessary to recompute the root Cid.

Follows on from #103 (comment)

The new model requires querying balance and allowance data separately after every mint or transfer is completed, in order to deal with possible actions taken inside the receiver hook. If state didn't change inside the receiver hook call, we could safely use the values we get from within the mint/transfer implementations.

It should be easy to pass through those values in the intermediate data so they are ready for use, the tricky part here is knowing when the state is 'dirty' and we need to query fresh values. We could save the root CID in there as part of making the hook call and compare it afterwards (in mint_return) to decide but maybe there's a better way to do it?

I added these to avoid build errors with unresolved externs in non-WASM builds because the SDK syscalls pull them in under the hood and they won't be considered dead code if we're using anything touching the hashing or method call code. A better long term solution might be to integrate with the new fvm_integration_tests package instead, or find some other means of dealing with them at the SDK level.

@anorth has suggested

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.

Splitting these up gives the user an opportunity to save (and commit) their state before calling the receiver hook, to guard against re-entrance attacks. The initial split is simple enough - make the mint/transfer functions return suitable parameters to forward to the receiver hook - but ideally we'd have it return some callable type that required the user to call the hook or abort the transaction entirely.

First part has already been implemented (#76), still need to add a callable type to enforce the receiver hook call and a number of tests will need to be adapted or rewritten for the new design

We would like to write example actors that consume the fil_token package and test the behaviour at the actor level

Currently actors within the ref-fvm repo are tested via the internal fvm_integration_tests package. However, attempting to use that here causes compilation errors.

The existing impl From<TokenError> for ExitCode consumes the error value. This makes it slightly annoying difficult to both get an exit code and invoke to_string() or do any other inspection of the error value.

The built-in actors use two statements to first inspect the message, then get the exit code at the last moment:

    fn actor_result(self) -> Result<T, ActorError> {
        self.map_err(|e| {
            let msg = e.to_string();
            ActorError::unchecked(ExitCode::from(e), msg)
        })
    }

It would be nice to find a way to get a code from a reference.

See the built-in actors and filecoin-project/builtin-actors#513 (comment) as inspiration.

Add a method to TokenState that checks all invariants. Call this at the end of every unit test, and expose it for use externally.

Some token library tests were ignored in #55 when the token receiver hook call was extracted. Move or rework the tests so that none are ignored.

Discussions of dropping anyhow in some of our dependencies may warrant re-evaluating if we want to opt our consumers in to this
filecoin-project/builtin-actors#214
filecoin-project/ref-fvm#463

See the FRC. Add the method to the library, and appropriate types/aliases, and to the example actor.

#75 (comment)

Looks like a CI run ends up building everything two or three times, which takes ages on the GitHub action runners. We should look into improving this when time allows. Might be time to split into multiple workspaces (eg: library vs actor code) or if there's enough actor code, maybe a separate repo?

Could be some easy wins running some build stages in parallel too?

The method hash macro uses blake2b-256, which is different than the FVM version (blake2b-512), changed in #89.

When fixing this, please create some "golden" tests for both the FVM and macro versions that include some hardcoded expected method number results that are computed via some separate code written in another programming lanuage.

For reverting state after hook call, benchmark the following strategies

Clone and Restore

1. Clone state in memory
2. Make changes
3. Flush
4. Call Hook
5. Restore cloned state in memory
6. Flush

Reload from Cid

1. Store Cid
2. Make changes
3. Flush
4. Call Hook
5. Reload from Cid into memory

In the built-in actors, callers currently do something like

                token
                    .mint(operator, to, &params.amount, RawBytes::default())
                    .exit_code(ExitCode::USR_UNSPECIFIED) // FIXME translate error to exit code

(where exit_code is method on Result<T, std::error::Error> that adapts to an ActorError (built-in actors type that carries exit code and results in abort).

Without doing anything too specific to the built-in actors runtime, add a standard mapping of errors to exit codes, so that all token implementers will use consistent code by default. A free function doing the mapping could be sufficient.

To investigate the possibility of having a single code bundle on-chain that can be constructed with different params.

Simplest would be to have an initial account which is permitted to mint that can then be permanently revoked. Or simply a fixed supply to begin with distributed to specified account(s).

The current address initialization/resolution logic has a few warts:

• resolve_id doesn't guarantee that the ID address is actually assigned. This isn't a huge issue, but can be very confusing.
• With FIP0048, initialize_account is no longer accurate. It will need to be something like initialize_address.
• Users should never call initialize_address/initialize_account. They should instead call resolve_or_create/resolve_or_init.

IMO, we should have two methods:

• resolve_existing that asserts that the target ID actually exists.
• resolve_or_init that tries to create the target ID.

We can also just rename initialize_account to initialize_address...

Given that this is shaping up to be a core shared library, it should live in the filecoin-project org:

• Either as a sub-crate in the builtin actors.
• Or as a new package? Maybe something like librfc?....
• Or something in the ref-fvm repo?

Right now, there's an awkward dependency detour where:

• The builtin actors depend on this repo.
• This repo depends on the FVM.

Most changes to the FVM's API will require a change change in this repo before that change can be propagated to the builtin actors. This will be a non-issue once M2.2 (general programmability) ships as the FVM interfaces will have stabilized by then (or we'll have proper FVM "container" versioning, at the very least). But the current situation won't work for M2.1 and M2.2 because we're actively changing said interfaces.

See filecoin-project/FIPs#435

The fungible token state uses ActorIDs directly as keys in the HAMTs in its state. This is not spec compliant because ActorIDs serialize as cbor ints. This is something that the HAMT library should have caught with type checking but unfortunately this is not yet the case (filecoin-project/ref-fvm#900).

For now the right way to handle this is to wrap integer serializing keys in a byte serializing type here's an example in the power actor.

See filecoin-project/FIPs#445

You'll need a new release of the FVM SDK to get the blake-512.