Implement the tez wrapper contract #255
Conversation
|
Now it should be complete, though it's pretty rough around the edges. There are a few things I need to fix before undrafting the PR. I am especially bothered by how specialized @dorranh could you also have a look and let me know if you see any red flags? I know it's still a draft but I could use a second pair of eyes 😅 |
| (* 3. Create a vault, if it does not exist already, and update the map. *) | ||
| let op_opt, state, vault_address = find_vault_address state !Ligo.Tezos.sender in | ||
| (* 4. Instruct the vault to send the actual tez to the owner *) | ||
| let op = match (LigoOp.Tezos.get_entrypoint_opt "%vault_send_tez_to_contract" vault_address : (Ligo.tez * Ligo.address) Ligo.contract option) with |
There was a problem hiding this comment.
I'm wondering if we want to send the tez to a contract or via a bare transaction here to enable withdrawals directly to user accounts? Not sure if there are any negative implications to doing this though
| | None -> ([op], state) | ||
| | Some origination -> ([origination; op], state) | ||
|
|
||
| let[@inline] set_delegate (state: tez_wrapper_state) (kho: Ligo.key_hash option) : LigoOp.operation list * tez_wrapper_state = |
There was a problem hiding this comment.
I'm not sure if we need to create the vault if it doesn't exist when calling this entrypoint, but it does at least make the behavior consistent with the FA2 entrypoints
There was a problem hiding this comment.
That's a fair point, we could also fail without any problems. I mostly tried to make vault origination implicit (and eager) everywhere, so that, indeed, similarly to FA2, operations work fine even if an account is not created yet or if a user has zero tez, etc. I guess this approach is rather eager when it comes to vault creation, but I think that's actually nice: we never have to fail due to "vault non-existent" errors. I can't think of a downside at the moment but perhaps we should revisit the approach.
| | None -> ops (* vault is already originated *) | ||
| | Some origination -> (origination :: ops) in (* originate now *) (* NOTE: reversed! *) | ||
|
|
||
| (* TODO: Do we really need to restrict the to_ and from_ here? *) |
There was a problem hiding this comment.
What I worry about with this is that we could end up causing vaults to be created for vaults if someone transfers tez-tokens to a vault address. I'm not sure if this causes vulnerabilities, but it feels a bit odd.
e.g.
I transfer to address of Vault A -> No record exists in ledger or Vault A -> Vault for Vault A is originated and transfer is executed
At that point the tokens are lost to the vault since we can't issue transfers from Vault A directly, though I guess the end result is just that the tokens are lost (not much different than sending tokens to an unknown address)
There was a problem hiding this comment.
Ah, I see what you mean. Yes, that could happen but indeed it's not much different than sending tokens to an unknown address. I'm sure that it cannot be used for an attack since we're using bigmaps (the performance of the tez wrapper will not degrade no matter how many vaults someone can create), but I guess it is a little weird. On the other hand, adding the check will raise gas costs, but people will still be able to create vaults for useless accounts if they want so the problem is not entirely gone. 🤔
There was a problem hiding this comment.
Hmmm, looking more into this, I think that implementing a check about whether an address belongs to a vault is more complicated than it seems at first glance: the big map is indexed by user address, so figuring out whether an address is a vault address cannot be done unless we keep track of them in a separate map. I added a check for !Ligo.Tezos.self_address for now, but we might have to reconvene about the vault check.
There was a problem hiding this comment.
Ah true, I hadn't thought about that. Given that we're trying to avoid gas costs I agree that it might make more sense to prioritize that over user-friendliness here since allowing users to create extra vault contracts shouldn't affect the functionality of the wrapper.
gkaracha
force-pushed
the
GEN
branch
5 times, most recently
from
f0e639b to
7e6aa28
Compare
Yeah, totally. I didn't add any since now it's not called at all, but after #213 is completed we should be able to add some meaningful tests. |
|
|
Ah, the message bot is back to work, thanks @dorranh! |
It's alive! 🧑🔬 |
The problem is that we have checker-specific logic in fa2Interface.ml so we cannot really reuse it for the tezWrapper (e.g., the token_ids used are checker-specific, no vaults are involved, ..). Ideally, I'd keep fa2Interface.ml clean (i.e., leave only the types and basic infrastructure in it), and have a separate module (e.g., fa2State or something) with checker-specific data. This way, tezWrapper can open only fa2Interface and implement the specialized functions that it needs.
…s break things...
|
As described in #213 (comment). Currently WIP.
In addition to adding a draft of the tez wrapper contract, this PR performs some general changes as well.
The contents of
fa2Interface.mlare partitioned into three files:fa2Interface.mlcontains all user-facing FA2-conforming types (general purpose)fa2Ledger.mlcontains the definition offa2_stateand functions operating on it (general purpose)fa2Implementation.mlcontains the implementations of the three FA2 entrypoints (fa2_run_balance_of,fa2_run_update_operators, andfa2_run_transfer) (checker-specific).The reason for this partitioning is so that we can reuse the common parts (API in
fa2Interface.mland generic infrastructure infa2Ledger.ml) for the tez wrapper contract, while allowing the tez wrapper contract to implement its own versions of the FA2 entrypoints (which are indeed different than those of checker).NOTE: All three modules are tested in
testFa2.ml.Inline pragmas are added to three functions (
set_fa2_ledger_value,ledger_issue, andledger_withdraw) to avoid gas explosion. Ideally we would not inline any of these functions, but the order of definitions in ligo programs can have a (surprisingly) serious impact on gas costs. The partitioning above required some reordering of definitions which led to failures, hence the inline pragmas. This makes some sense to me since I expect the stack to be different, but such an effect on gas costs feels like a LIGO bug to me.