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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e91886c
wip
rsoeldner May 2, 2023
4d0cff8
add detailed description
rsoeldner May 4, 2023
b590ada
wip
rsoeldner May 4, 2023
e651a76
address comments
rsoeldner May 5, 2023
a6d47f0
Apply suggestions from code review
rsoeldner May 8, 2023
b49dbbc
address review comments
rsoeldner May 8, 2023
7310398
Refine motivation
rsoeldner May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
93 changes: 93 additions & 0 deletions kip-0019.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
---
KIP: 0019
Title: Coin-v6 and fungible-v3
Author: Robert Soeldner (robert@kadena.io)
Status: Draft
Type: Standard
Category:
Created: 2023-05-02
---

## Abstract

The next iteration of the coin contract proposes three main changes to enhance
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 •
edited

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 •
edited

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.

Copy link

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.


Secondly, the constants and some functions within the contracts will be reorganized to
increase maintainability (see [Rationale](#rationale)). This is intended to make the codebase more structured and easier
to maintain, reducing the chances of errors and improving the overall quality of the contract.

Finally, verification related code of the coin contract will be modified to allow for
formal verification. This will enable rigorous proves of the contract's code to ensure
its correctness and reliability. By incorporating these changes, the new iteration of the
coin contract aims to provide a more secure and efficient platform for the exchange of
cryptocurrencies.

## 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 •
edited

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 •
edited

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.

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.
Copy link

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.


The removal of `rotate` will _not_ remove the ability for accounts to have Rotatableg 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.

Moreover, the utilization of our formal verification system can significantly enhance the
security of the contract. Therefore, necessary changes to allow the verification of the contract
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,

during which participants are expected to share their feedback.

| 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 |



## Rationale
Changes within `fungible-v3.pact`:
* Model annotation for the schema `account-details`: Add new invariant expressing positive
balance (`>= balance 0.0`).
* Model annotation for defun `get-balance`: Add new property that `!= account ""`
and `>= result 0.0`.
* Model annotation for defun `details`: Add property that the account is not empty `!= account ""`.
* Model annotation for defun `enforce-unit`: Add property that the amount is
not negative `>= amount 0.0`.
* Model annotation for defun `create-account`: Add property that the account is not
empty `!= account ""`.

Changes within `coin-v6.pact`:
* Change the `fungible-v2` interface to `fungible-v3`.
* Bless old coin contract (coin v5).
* Move constants (`COIN_CHARSET`, `MINIMUM_PRECISION`, `MINIMUM_ACCOUNT_LENGTH`,
`MAXIMUM_ACCOUNT_LENGTH`, and `VALID_CHAIN_IDS`) to a different section.
* Move utility functions (`enforce-unit`, and `validate-account`) to a different section.
* Replace `enforce (!= sender ...` by `validate-account` in defun `DEBIT` and `CREDIT`.
* Remove defcap `ROTATE`
* Add additional `enforce` to express that gas consumtion must be greater than
zero (`enforce (> total 0.0)`) within defun `redeem-gas`.
* Enforce the refund unit `enforce-unit refund` within defun `redeem-gas`.
* Add `validate-account` to defun `get-balance`.
* Add `validate-account` to defun `details`.
* Within `fund-tx` enforce a valid miner within the `defpact` step.
* Add invariant `>= balance 0.0` for the `allocation-schema`.

## Backwards Compatibility

N/A

## Specification

* [coin-v6.pact](kip-0019/coin-v6.pact)
* [coin-v6.repl](kip-0019/coin-v6.repl)
* [fungible-v3.pact](fungible-v3.pact)