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

KIP-0019: coin-v6 and fungible-v3 #43

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Conversation

rsoeldner
Copy link
Member

@rsoeldner rsoeldner commented May 2, 2023

Rendered version: https://github.com/kadena-io/KIPs/blob/b49dbbcf021cab75e71c6e15cf08c2ef23b2db0c/kip-0019.md

TODO:

  • Timebox proposal
  • Check gas consumption in coin-v6.repl

@rsoeldner rsoeldner changed the title KIP-0019: Coin-v6 KIP-0019: coin-v6 and fungible-v3 May 4, 2023

## Motivation

The presence of the `rotate` function in the current implementation of the contract undermines the
Copy link

Choose a reason for hiding this comment

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

I highly recommend not to remove rotate as this is one of our unique selling points over other chains where the account/address is directly tied to the public/private keypair. If your private key get's compromised on such chain you are forced to move all existing assets to a new address(keypair).

Kadena's chain has the unique property that our address is not tied to the keypair (or keyset in our case). So being able to rotate is a huge benefit. Allowing users to keep the same address, but changing their keyset to sign with.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @EnoF here that rotation is unique to the Pact language in that you can rotate the underlying governance of a particular account in the coin contract, but there is a distinction here that needs to be made:

The removal of rotate will not remove the ability for accounts to have rotatable governance. This is a misconception. It will remove the ability to arbitrarily rotate account governance from any guard to any guard. However, the feature still remains that if one uses a keyset reference and stores a keyset-ref-guard, then keys are rotateable via define-keyset. Since keysets are namespaced, users may unambiguously name and store guards they expect to rotate, and the principal account type associated with them is the r: protocol, so they may be stored under unique account addresses as well.

So the question here that needs to be answered is this: should guards be arbitrarily rotatable, or should rotation be restricted to when it makes sense? rotate makes any crooss-chain transaction possibly unsound in its presence: funds can be locked up if any call to rotate occurs in between the burn and creation steps on the two chains. That makes for a terrible UX! I don't think restricting rotation to just builtin Pact primitives would create the same burden of UX because there already exists a recourse for users that care about rotation.

Copy link
Member

Choose a reason for hiding this comment

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

As an addendum: also, keep in mind that rotate breaks the invariants for principal account protocols. This is extremely important! While enforce-reserved in the coin contract requires that k: accounts be well-formed when doing any useful operation in the coin contract, one can still call rotate on principal accounts, effectively bricking them until they are rotated back to the correct governance defined by the principal. There are only negatives here: principals don't work as intended, rotate doesn't provide any value, and we're going to be restricting create-account so that users can only interact with coin if they have a principal account protocol in play. The existence of rotate in this case only introduces surface area for griefing, with no benefits.

Copy link

Choose a reason for hiding this comment

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

If I understood this correctly, this means that users should be encouraged to create accounts like:

(coin.create-account "k:mykey" (keyset-ref-guard "r:my-keyset"))

And then they will be able to rotate said keyset like:

(define-keyset "r:my-keyset" (read-keyset 'new-keyset))

Which then makes the coin.transfer-crosschain reference the defined keyset rather than the value of the keyset and therefore you get the SPV to become sound? If this is correct we should probably some more "encouraging documentation" around this for new devs. At least to me the use of define-keyset did not ring a bell to be used in this way.

Copy link
Member

@emilypi emilypi May 11, 2023

Choose a reason for hiding this comment

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

@EnoF close! You understood correctly, but for the interest of the KIP process, there are some details to iron out. Namely, let's look at the call you've written:

 (coin.create-account "k:mykey" (keyset-ref-guard "r:my-keyset"))

This would not be correct because the account name for an r: protocol account would be r:<my-keyset>, while the keyset ref guard would be called to wrap around a particular predefined keyset. In fact, the call to creating a rotatable account would be something like this in general:

(coin.create-account 
  (create-principal (keyset-ref-guard "<some keyset name>")) 
  (keyset-ref-guard "<some keyset name>"))

The call to create-principal creates an r:-principal account name much in the same sense as k:. They look like r:<some keyset name>. And yes, the underlying keyset is fully rotateable via calls like:

(define-keyset "<some keyset name>" (read-keyset 'new-keyset))

Which then makes the coin.transfer-crosschain reference the defined keyset rather than the value of the keyset and therefore you get the SPV to become sound? If this is correct we should probably some more "encouraging documentation" around this for new devs. At least to me the use of define-keyset did not ring a bell to be used in this way.

Yes, exactly. If the keyset is defined on the target chain, then the keys may rotate underneath it, but the keyset name reference is a static string, which does not change. We have documentation for this in the principal account section of the readthedocs, but I'll work on writing up a concrete tutorial/introduction and tie it in with this proposal to ease everyone's minds.

Copy link

Choose a reason for hiding this comment

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

@emilypi I tried creating an account using a keyset-ref-guard,
which requires me to use define-keyset. I was hoping to define it like:

(define-keyset
  (create-principal (read-keyset 'keyset))
  (read-keyset 'keyset))

In the hopes that the keyset would be guarded like a k:account would be.
This is not possible because you need to be on a namespace to use define-keyset.
Which means that I would use either free or user, my keyset will not be guarded.

Then I thought maybe I could use ns-create-principal-namespace to solve this problem:

(define-namespace
  (ns.create-principal-namespace (read-keyset 'keyset))
  (read-keyset 'keyset)
  (read-keyset 'keyset))

(namespace (ns.create-principal-namespace (read-keyset 'keyset)))

(define-keyset
  (format "{}.{}" [(ns.create-principal-namespace (read-keyset 'keyset )) "keyset"])
  (read-keyset 'keyset))

(coin.create-account 
  (create-principal 
    (keyset-ref-guard 
      (format 
        "{}.{}" 
        [(ns.create-principal-namespace (read-keyset 'keyset )) "keyset"])))
    (keyset-ref-guard 
      (format 
        "{}.{}" 
        [(ns.create-principal-namespace (read-keyset 'keyset )) "keyset"])))

This should prevent your keyset ref from being squatted, but it is quite a bit of boilerplate.

I think it would be a good compromise between UX/DX and security if we would
upgrade the create-account to auto define a keyset in a way that only the original
keyset can sign for. (i.e. k:pubkey the pubkey needs to sign for the initial
define-keyset). Maybe this wouldn't work on the define-keyset, but we could maybe
introduce something like ks.create-principal-keyset that would do this for you.

Copy link
Member Author

Choose a reason for hiding this comment

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

@EnoF I see your point using create-principal-namespace and I think define-keyset could implicitly use the current namespace. Maybe there are good reasons for explicitly writing the NS @emilypi ?

For later reference, repl script using the KIP-0019 infrastructure showing key-rotation.

(begin-tx)
(env-data {"robert": {"keys": ["key1"], "pred": "keys-any"}})
(env-sigs [{'key: "key1", 'caps: []}])

(define-namespace 'free (read-keyset "robert") (read-keyset "robert"))
(namespace 'free)
(define-keyset "free.robert" (read-keyset "robert"))
(commit-tx)


(begin-tx)
(namespace 'free)
(env-data {"andy": {"keys": ["key2"], "pred": "keys-any"}})
(env-sigs [{'key: "key2", 'caps: []}])

(expect-failure
 "cant rotate keys due to missing keyset"
 (define-keyset "free.robert" (read-keyset "andy")))
(commit-tx)

(begin-tx)
(namespace 'free)
(env-data {"robert": {"keys": ["key1"], "pred": "keys-any"},
             "andy": {"keys": ["key2"], "pred": "keys-any"}})
(env-sigs [{'key: "key2", 'caps: []} {'key: "key1", 'caps: []}])

(define-keyset "free.robert" (read-keyset "andy"))
(commit-tx)


(begin-tx)
(namespace 'free)
(load "fungible-xchain-v1.pact")
(load "fungible-v3.pact")
(load "coin-v6.pact")


(create-table coin.coin-table)
(create-table coin.allocation-table)

(coin.create-account
  (create-principal (keyset-ref-guard "free.robert"))
  (keyset-ref-guard "free.robert"))
(commit-tx)

Copy link

@EnoF EnoF May 22, 2023

Choose a reason for hiding this comment

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

Note that free.robert can be squated by anyone on a different chain. So you could have defined free.robert with your keyset on chain 1, while I would define free.robert on chain 2 with a different keyset.

So to prevent this from happening, we need to have define-keyset enforce the initial keyset used to create the definition, i.e. k:pubkey or a different principal that could enforce such requirement.

@EnoF
Copy link

EnoF commented May 4, 2023

I'd suggest to update the create-account to have a creation-key-set to encourage users to create their k:account with a different keyset compared to the keysets used to sign for. This being that if your private key gets compromised that was used to create the k:account you risk getting your account squatted on chains you haven't created your account yet.

So something along the lines of:

(defun create-account(creation-keyset:guard sign-keyset:guard)
  (let ((account (create-principal creation-keyset)))
    (enforce-keyset create-keyset)
    (enforce-keyset sign-keyset)
    (insert coin-table account
      { balance: 0.0
      , guard: sign-keyset })))


The presence of the `rotate` function in the current implementation of the contract undermines the
integrity of cross-chain transfers. Specifically, executing the rotate function before the
completion of a cross-chain transfer can result in the locking of funds.

Choose a reason for hiding this comment

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

It may be nice here to explain why the funds would be locked. i.e. "the cross chain transfer is permanently tied to a given guard, and rotating the keys of the coin contract makes that saved guard invalid."

It may also be good here to explain the recommended workaround: migrating to keyset references before the update to coin contract.

kip-0019.md Outdated Show resolved Hide resolved
kip-0019.md Outdated Show resolved Hide resolved
kip-0019.md Outdated Show resolved Hide resolved
kip-0019.md Outdated Show resolved Hide resolved
kip-0019.md Outdated Show resolved Hide resolved
kip-0019.md Outdated Show resolved Hide resolved
kip-0019.md Outdated Show resolved Hide resolved

## Motivation

The presence of the `rotate` function in the current implementation of the contract undermines the
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @EnoF here that rotation is unique to the Pact language in that you can rotate the underlying governance of a particular account in the coin contract, but there is a distinction here that needs to be made:

The removal of rotate will not remove the ability for accounts to have rotatable governance. This is a misconception. It will remove the ability to arbitrarily rotate account governance from any guard to any guard. However, the feature still remains that if one uses a keyset reference and stores a keyset-ref-guard, then keys are rotateable via define-keyset. Since keysets are namespaced, users may unambiguously name and store guards they expect to rotate, and the principal account type associated with them is the r: protocol, so they may be stored under unique account addresses as well.

So the question here that needs to be answered is this: should guards be arbitrarily rotatable, or should rotation be restricted to when it makes sense? rotate makes any crooss-chain transaction possibly unsound in its presence: funds can be locked up if any call to rotate occurs in between the burn and creation steps on the two chains. That makes for a terrible UX! I don't think restricting rotation to just builtin Pact primitives would create the same burden of UX because there already exists a recourse for users that care about rotation.

rsoeldner and others added 2 commits May 8, 2023 19:33
Co-authored-by: Emily Pillmore <emily@kadena.io>
the _maintainability_ and _security_ of the contract.

Firstly, the `rotate` function, which is responsible rotating an account guard in a contract that implements the `fungible-v2` interface,
will be removed. In the case of fungible tokens that also implement the `fungible-xchain` interface, this will result "correct by construction" cross-chain transactions by avoiding the risk of locking funds sent to the receiving account.
Copy link

Choose a reason for hiding this comment

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

you don't need to rotate to get funds stuck in cross chain. Only specifying the wrong receiver keyset is enough. You can specify any arbitrary keyset (correct me if wrong), the sending chain can't check if the account exists on the receiving chain and with which keyset it was created. getting stuck doing rotate during crosschain is a special case, i'm not sure that happens in the real world..

Copy link

Choose a reason for hiding this comment

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

rotate saved me a few times from such errors. if I specified the wrong keyset during cross chain but still a keyset I have control of. It would fail on the second part but could fix it by rotating the receiver to the matching key when finding out the mistake. Then attempt the continuation part again.

Copy link

Choose a reason for hiding this comment

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

the only way that removing rotate can contribute to eliminating the risk on stuck cross-chain is if you are going to disable cross-chain on non-principal -classic arbitrary- account names and validate-principal for the principal account name in the first initial step of the defpact for cross-chain. Sorry if that is already implied in the KIP here, I might have missed it then..

Copy link
Member

Choose a reason for hiding this comment

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

you don't need to rotate to get funds stuck in cross chain. Only specifying the wrong receiver keyset is enough. You can specify any arbitrary keyset (correct me if wrong), the sending chain can't check if the account exists on the receiving chain and with which keyset it was created. getting stuck doing rotate during crosschain is a special case, i'm not sure that happens in the real world..

rotate saved me a few times from such errors. if I specified the wrong keyset during cross chain but still a keyset I have control of. It would fail on the second part but could fix it by rotating the receiver to the matching key when finding out the mistake. Then attempt the continuation part again.

the only way that removing rotate can contribute to eliminating the risk on stuck cross-chain is if you are going to disable cross-chain on non-principal -classic arbitrary- account names and validate-principal for the principal account name in the first initial step of the defpact for cross-chain. Sorry if that is already implied in the KIP here, I might have missed it then..

Just to be clear here, we're not trying to make the blockchain your mother. If you type in the wrong receiver or their governance details, you will lose your funds unless you have a means of recovery available, like you own the keys and you simply sent to the wrong one of your many accounts. The problems you describe are endemic of any call to a transfer in which you fat finger details. We cannot and do not want to guard you against unforced errors like what you describe - you must take reasonable care. However, we can protect against nondeterministic problems such as the person you're sending to being able to rotate in between your crosschain transfers.

And yes, there is another KIP out to enforce that create-account from the next coin version onward only works for principal account creation.

Copy link
Member Author

@rsoeldner rsoeldner May 13, 2023

Choose a reason for hiding this comment

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

@trendzetter I'm very happy you brought up this point and started interacting in this discussion. As Emily said, we currently do not protect against typing a wrong receiver, but this KIP aims to increase the security. The additional model annotations will provide more secure properties and trust into the contract.

(I'm on my way creating the new KIP for the create-account restriction.)

Copy link
Member

@emilypi emilypi May 13, 2023

Choose a reason for hiding this comment

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

To add color to @rsoeldner - no blockchain does protect you against these things. There's always the possibility that a) you fail to transfer because you've used coin.transfer, or b) you fat fingered a coin.transfer-create, which is not considerd a "safe" transfer for this exact reason. We offer remedies for this:

  1. The /local endpoint, which can simulate transactions before you execute the on-chain transaction. Wallets are always encouraged to use this to confirm details for both the data query side (receiver exists, guards match etc.), and the simulated transaction execution side in which the transaction is run against the most recent blockchain state. Automating this workflow is standard.

  2. The signing api (under construction)

I can only see people running into the problem of fat-fingering if they either execute their own queries and fail to check their details beforehand, or if they're using transfer scripts that fail to do so. To be clear: I agree that your problems are indeed real UX problems, but it's something wallets and dApp developers need to attack using the existing tools. Deferring to the base protocol to enforce them is overly-paternal IMO, and a divergence in the nature of the blockchain.

Choose a reason for hiding this comment

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

I think there is in itself nothing wrong with protecting people from destroying their coins if it doesn't have an unacceptable cost. I was triggered into responding thinking that removing rotate in itself wouldn't improve the overall situation much, not nostalgia for rotate or dangerous legacy style accounts (yes it exists!)
But given this is part of a salami with more slices I think it might be feasible and desirable to make it impossible to destroy coins, and make it extremely unlikely you accidentally send the coins to a random other valid account (valid principal accounts are less likely to be found by accident). So I am looking forward to the changes on create-account (transfer-create&transfer-crosschain?). Old style accounts can then be safely used to migrate out through simple transfer to a principal account name.

are introduced.

## Timeline
The KIP process has a time constraint of *1.5 months*, which will end on June 30th AoE,
Copy link
Member Author

Choose a reason for hiding this comment

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

We extended the time constraint until mid July:

Suggested change
The KIP process has a time constraint of *1.5 months*, which will end on June 30th AoE,
The KIP process has a time constraint of *1.5 months*, which will end on July 30th AoE,

| Date | Event |
|------------|-------------------------------- |
| 2023-05-02 | Publication of initial document |
| 2023-06-30 | Deadline of KIP-0019 |
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
| 2023-06-30 | Deadline of KIP-0019 |
| 2023-06-15 | Publish [medium article](https://medium.com/kadena-io/kip-0019-future-iteration-of-the-coin-and-fungible-contracts-56ba5b6360e1) |
| 2023-07-15 | Deadline of KIP-0019 |

@CryptoPascal31
Copy link

I have a big concern about the fungible-v3 interface.

I believe that this will break all contracts relying on coin used with a modref. Like existing DEXs.

I am wrong ?

Is it worth upgrading fungible-v2 -> funglibe-v3 just for adding some not-essential FV tests and removing (rotate ).

Furthermore (rotate ) can still be disabled in coin-v6 while keeping funglible-v2

@emilypi
Copy link
Member

emilypi commented Jun 16, 2023

@CryptoPascal31 Interfaces are not upgradeable by design, so that's the main cause of all of this kerfuffle.

@CryptoPascal31
Copy link

@CryptoPascal31 Interfaces are not upgradeable by design, so that's the main cause of all of this kerfuffle.

Yes I know..

I maybe wrong but that's why, I think that changing the interface implemented by coin must be avoided if not absolutely necessary. All contracts using coin as a modref will be broken.

And even worse, DEX contracts will need to be rewritten in an "hybrid way" to be able to handle both interfaces: funglible-v2 for old tokens and fungible-v3 for coin => Nightmare

@Thanos420NoScope
Copy link

This cannot go through in its current state. A blockchain should never break previously deployed contracts. Think of the financial implications of this. People and developers will lose trust in your network if you cause this kind of damage. You cannot expect everyone to upgrade their contracts to "keep working". This change to fungible-v3 could cause major headaches to the ecosystem.
Thank you Pascal for pointing this out.

@michaeldoron59
Copy link

michaeldoron59 commented Jun 19, 2023

Hey guys do you think it will be possible to wrap tokens with some fungiblev2->fungiblev3 wrapper contract or something similar so the code won't break and this way everyone's happy?
Another option is to make the opposite way so we can downgrade coin to v2 in some places like dexes, so the rotation will be asserted as not implemented or something.. I'm not sure that we need to break everything in the current state.

@rsoeldner
Copy link
Member Author

@CryptoPascal31 @Thanos420NoScope @michaeldoron59 thank you for your valuable feedback.
We are discussing the mentioned points to make sure these concerns are adequately addressed.

@jmcardon
Copy link
Member

This cannot go through in its current state. A blockchain should never break previously deployed contracts. Think of the financial implications of this. People and developers will lose trust in your network if you cause this kind of damage. You cannot expect everyone to upgrade their contracts to "keep working". This change to fungible-v3 could cause major headaches to the ecosystem. Thank you Pascal for pointing this out.

We will not break fungible-v2 nor remove it from coin-v6. We are aware of how breaking this is to the ecosystem, and it's important the DEXs keep working.

I just want to point out that the KIP is precisely for these discussions. We discuss these things internally as well, but in an effort to bring about more transparency, we're simply moving a lot of these discussions out to a public forum.

@CryptoPascal31
Copy link

We will not break fungible-v2 nor remove it from coin-v6.

When I look at the files attached in this PR, coin-v6 is not implementing fungible-v2 anymore but fungible-v3.

I may be wrong, but in my understanding, if deployed as it is, this will technically break all DEXs after the next fork.

Do you agree @jmcardon ?

@michaeldoron59
Copy link

We will not break fungible-v2 nor remove it from coin-v6.

When I look at the files attached in this PR, coin-v6 is not implementing fungible-v2 anymore but fungible-v3.

I may be wrong, but in my understanding, if deployed as it is, this will technically break all DEXs after the next fork.

Do you agree @jmcardon ?

IMO it will just break future lp creations, but not old ones. but yeah indeed it's still a massive headache to the whole ecosystem.

@CryptoPascal31
Copy link

IMO it will just break future lp creations, but not old ones. but yeah indeed it's still a massive headache to the whole ecosystem.

No it won't work even for old pairs. For example kdSwap public functions expect modrefs as arguments implementing fungible-v2
But with the current proposal, coin will be a fungible-v3 and Pact will refuse to call a swap function because of types mismatch.

@peppinho89
Copy link

IMO it will just break future lp creations, but not old ones. but yeah indeed it's still a massive headache to the whole ecosystem.

No it won't work even for old pairs. For example kdSwap public functions expect modrefs as arguments implementing fungible-v2 But with the current proposal, coin will be a fungible-v3 and Pact will refuse to call a swap function because of types mismatch.

I can confirm. As is it will not work for both old and new pairs in all the DEXs. If coin will be updated implementing fungible-v3 it will affect all the functionalities across the DEXs (swap, add/remove liquidity etc).

Looking at the differences between fungible-v2 and fungible-v3, honestly I think it's not worth the effort just for add a couple of FV tests and remove the rotate function from the interface.

If the main reason is removing the rotate function I think the strategy to consider in this case it's just to disable the function in the coin contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants