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 3 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
75 changes: 75 additions & 0 deletions kip-0019.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
---
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,
rsoeldner marked this conversation as resolved.
Show resolved Hide resolved
will be removed to preserve cross-chain soundness. This is to avoid the risk locking funds.
rsoeldner marked this conversation as resolved.
Show resolved Hide resolved
rsoeldner marked this conversation as resolved.
Show resolved Hide resolved

Secondly, the constants and some functions within the contract will be reorganized to
increase maintainability. This is intended to make the codebase more structured and easier
rsoeldner marked this conversation as resolved.
Show resolved Hide resolved
rsoeldner marked this conversation as resolved.
Show resolved Hide resolved
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.


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.

## 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 of defcap `ROTATE`
rsoeldner marked this conversation as resolved.
Show resolved Hide resolved
* Add additional `enforce` to express that gas consumtion must be greater
rsoeldner marked this conversation as resolved.
Show resolved Hide resolved
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)