[TCFMM-10] Add FA2 entrypoints to manage positions

[rinn7e]

Description

Problem: Currently, the contract lacks support for FA2 entrypoints.
We should add this.

Solution: Add support for FA2 entrypoints: transer, balance_of,
and update_operator.

Related issue(s)

Resolves TCFMM-10
Depends on #8

✅ Checklist for your Pull Request

Related changes (conditional)

• Tests
  • If I added new functionality, I added tests covering it.
  • If I fixed a bug, I added a regression test to prevent the bug from
    silently reappearing again.

• Documentation
  • I checked whether I should update the docs and did so if necessary:
    • README
    • Haddock

Stylistic guide (mandatory)

• My commits comply with the policy used in Serokell.
• My code complies with the style guide.

[rinn7e]

@dcastro @Martoon-00 This PR is ready for review. A few things to note/ask is that:

• I've removed admin, we probably tackled this in a separate issue.
• Currently, we tracked the owner of a position in 2 place: positions and ledger. This requires updating both of the map when we change the owner. I'm thinking of combining them somehow, let me know if what you think about this.
• Related to error message, should we turn FA2 error message to error code as well or not.

[Martoon-00]

Generally looks good to me 👍

Only one thing that worries me: swap_position effectively performs many of the checks that are already performed in transfer_item.

I think we should remove duplications in one function or another, and in swap_position state clearly what preconditions should hold upon calling it (e.g. whether amt must be 1, 0 or 1, or do not mention anything if we completely check amt inside).

[Martoon-00]

Regarding your points, I personally think everything is properly done now.

[rinn7e]

I think we should remove duplications in one function or another.

@Martoon-00 Make sense, I've move all the amt check to swap_position, and rename this function to change_position_owner instead to avoid confusion. Let me know what you think about this.

[Martoon-00]

Looks good to me 👍

The last point: since you already seemed to rebase on top of the Errors PR, shouldn't you replace failwith "impossible" with throwing nat codes? Or you left this for the moment when the Errors PR gets merged?

[rinn7e]

Looks good to me 👍

The last point: since you already seemed to rebase on top of the Errors PR, shouldn't you replace failwith "impossible" with throwing nat codes? Or you left this for the moment when the Errors PR gets merged?

Since FA2 has its own error standard, should we turn it into error nat or not? It may not play well with other FA2 contract client (Not sure if it is that important or not anyway). What do you think about this?

[Martoon-00]

Looks good to me +1
The last point: since you already seemed to rebase on top of the Errors PR, shouldn't you replace failwith "impossible" with throwing nat codes? Or you left this for the moment when the Errors PR gets merged?

Since FA2 has its own error standard, should we turn it into error nat or not? It may not play well with other FA2 contract client (Not sure if it is that important or not anyway). What do you think about this?

It's indeed tricky in general case, but I think for internal errors we are free to choose any desired format.

[rinn7e]

It's indeed tricky in general case, but I think for internal errors we are free to choose any desired format.

My bad, I thought you referred to FA2 errors 🤦. I've updated this now.

[Martoon-00]

Now looks great 👍

Thanks for addressing my comments.

[Martoon-00]

Sorry for the later rereview, left a couple of minor comments.

The change of the structure looks good, nice that it could be made simpler.

[Martoon-00]

This check is also better to be documented I think. AFAIU FA2 does not mention that Update_operators can throw an error when token_id does not yet exist.

[rinn7e]

@Martoon-00 Strange that in tzip-12, Update_operators does not throw error for that. Do you know any reason why? Should we adhere to the tzip-12 (no error thrown, just leave storage not updated) or keep the error but update our spec?

[Martoon-00]

I'm not sure too. I remember one contract that allowed a user to reserve a sequence of token_ids and later mint money at them, for such a contract it could be reasonable to allow setting operators in advance.

For our contract, setting an operator in advance does not make sense I think, so throwing an error seems justified.

[rinn7e]

I see. In our use case, when setting an operator of an owner, that means he can transfer any of the owner positions right? In this case, ensuring a position exist or not may not be necessary.