vyperlang / vyper

Pythonic Smart Contract Language for the EVM
https://vyperlang.org
Other
4.92k stars 806 forks source link

VIP: native asset types #3716

Open charles-cooper opened 11 months ago

charles-cooper commented 11 months ago

Simple Summary

bring new types to vyper which safely model assets, natively

Motivation

accounting is hard. instead of assets being represented by raw numbers as they currently are, add a special kind of asset type which is more constrained. this removes the possibility for rounding errors and will increase clarity of user code.

this new type enforces the invariant that each action to the ledger has an equal and opposite reaction. in code- terms, it un-denormalizes code, increases DRYness and reduces the potential for accounting bugs (ex. rounding errors, missed actions).

# example buggy code
def mint10():
    self.balances[addr] += 10
    self.totalSupply += 19  # typo
# with assets
def mint10():
    self.balances[addr] += 10   # error! can't assign directly
    self.balances[addr] = 10   # error! can't assign directly

    self.balances.move_from(self.totalSupply, 10)

notes:

Specification

add a new parametrizable type, Asset to vyper. Asset takes two type parameters, the subtype and an "intrinsic sign", which basically just corresponds to whether the account is debit- or credit- normal.

Asset cannot be assigned to directly, but can only be modified through the builtin move_from() method. (if you squint closely, move_from() is basically a single, balancing debit+credit).

example:

totalSupply: Asset[uint256, -]  # move_from totalSupply *increases* totalSupply
balances: HashMap[address, Asset[uint256, +]  # move_from a balance *decreases* the balance

def transfer(recipient: addr, amount: uint256):
    self.balances[recipient].move_from(self.balances[msg.sender], amount)

def mint(recipient: addr, amount: uint256):
    self.balances[recipient].move_from(self.totalSupply, amount)

tbd:

Backwards Compatibility

no breaking changes

References

https://github.com/vyperlang/vyper/issues/1277 https://en.wikipedia.org/wiki/Debits_and_credits

Copyright

Copyright and related rights waived via CC0

pcaversaccio commented 11 months ago

Overall I like this idea very much. But I have to admit that I had to read the specs multiple times to understand the semantics of the "intrinsic sign" correctly. I think the word move can also be debated. Like asset creations (i.e. what you refer to minting above) could be called create_resource(...) and destroying/removing assets could be simply called destroy_resource(...) (see below my naming suggestion). For moving the asset, it could be simply transfer_resource(...). This wording is much faster to comprehend. Also, what do you think about this syntax instead (I implicitly want to kick off the convo around generics):

T = vyper.TypeVar("uint256")
totalSupply: public(Resource[T])
balanceOf: public(HashMap[address, Resource[vyper.type(self.totalSupply)])

def transfer(to: address, amount: uint256):
    transfer_resource(self.totalSupply, self.balanceOf[msg.sender], self.balanceOf[to], amount)

def mint(owner: address, amount: uint256):
    create_resource(self.totalSupply, empty(address), self.balanceOf[owner], amount)

def burn(owner: address, amount: uint256):
    destroy_resource(self.totalSupply, self.balanceOf[owner], empty(address), amount)

So the functions would be like:

transfer_resource(resource: Resource[T], resource_origin: HashMap[address, Resource[vyper.type(resource)]], resource_destination: HashMap[address, Resource[vyper.type(resource)]], resource_amount: uint256)
create_resource(resource: Resource[T], resource_origin: address=empty(address), resource_destination: HashMap[address, Resource[vyper.type(resource)]], resource_amount: uint256)
destroy_resource(resource: Resource[T], resource_origin: HashMap[address, Resource[vyper.type(resource)]], resource_destination: address=empty(address), resource_amount: uint256)

We might want to have an unsafe version of it for people who wanna skip the compiler invariant checks to save gas and assume they know what they do :). Maybe this can be implemented via a kwarg...

I personally like Resource as a name since it's somehow more general. Asset is very finger-pointing to DeFi somehow, which is fine, but we should consider a name that is more generic IMO.

charles-cooper commented 11 months ago

i have a slight preference for "asset"- related terminology. "resource" sounds more like filehandles or linear types.

i think maybe the key insight from the "theory" of double-entry accounting being applied here is that assets are never created or destroyed, only moved -- and the way it is able to work is because some accounts have opposite intrinsic sign than others. so balances[addr].move_from(totalSupply) is a debit to totalSupply and a credit to balances, but that increases the value of both accounts. it enforces the invariant totalSupply - sum(balances) == 0 by construction!

charles-cooper commented 11 months ago

recommended reading for those unfamiliar with the debits/credits terminology: https://en.wikipedia.org/wiki/Debits_and_credits

fubuloubu commented 11 months ago

A totally different proposal that only requires struct methods to perform similar tasks:

# NOTE: Could be defined as an internal library type

struct Ledger(HashMap[address, uint256]):  # NOTE: could also add generic support over time
# struct Ledger[K: vyper.traits.Hashable, V: vyper.traits.AddSubTrait](HashMap[K, V]): ...

    # NOTE: We can add additional members to struct subclasses
    total: uint256

    # NOTE: `HashMap` subclasses `vyper.types.Mapping` and hashes the key for access
    # NOTE: `vyper.types.Mapping` defines two methods: `__getval__` and `__setval__`
    def mint(self, receiver: address, amount: uint256):  # NOTE: with generics, we could parametrize these inputs
        self.total += amount  # NOTE: This is safemath
        self.__setval__(
            receiver,
            # NOTE: This is safe because of the previous safe add
            unsafe_add(self.__getval__(receiver), amount),
        )

    def transfer(self, owner: address, receiver: address, amount: uint256):
        self.__setval__(
            owner,
            # NOTE: If underflow, then it should raise invalid operation
            self.__getval__(owner) - amount,
        )
        self.__setval__(
            receiver,
            # NOTE: This is safe because of the underflow check and the property of conservation on `.total`
            unsafe_add(self.__getval__(receiver), amount),
        )

    def burn(self, owner: address, amount: uint256):
        self.__setval__(
            owner,
            # NOTE: If underflow, then it should raise invalid operation
            self.__getval__(owner) - amount,
        )
        # NOTE: This is safe because of the underflow check and the property of conservation on `.total`
        self.total = unsafe_sub(self.total, amount),

    # NOTE: Should have someway of generating a `public` getter method

Then could be used like this:

from vyper.types import Ledger

balanceOf: public(Ledger)
# NOTE: export `totalSupply` public getter as `balanceOf.total`

@external
def transfer(receiver: address, amount: uint256) -> bool:
    self.balanceOf.transfer(msg.sender, receiver, amount)
    log Transfer(msg.sender, receiver, amount)
    return True

...

Kind of a nice side effect here is that the .total storage slot gets "flattened" into the struct

charles-cooper commented 11 months ago

A totally different proposal that only requires struct methods to perform similar tasks:

i think this is a fine proposal but it more belongs in a discussion about metaprogramming / dunder methods than the issue at hand

pcaversaccio commented 11 months ago

i think this is a fine proposal but it more belongs in a discussion about metaprogramming / dunder methods than the issue at hand

good point - we should actually have an open issue about these topics...

pcaversaccio commented 11 months ago

Some further thoughts. Maybe it's worth considering the dataclass decorator as we will add further special dunder functions in the future:

__typevars__ = { "T" }

@dataclass(getval=True, setval=True, system=False, typevars=True)
struct Ledger(HashMap[address, T]):
    total: T

    def mint(self, receiver: address, amount: T):
        self.total += amount
        __setval__(receiver, unsafe_add(__getval__(receiver), amount))
        __system__(b"...") # Does not work since disallowed

    def transfer(self, owner: address, receiver: address, amount: T):
        __setval__(owner, __getval__(owner) - amount)
        __setval__(receiver, unsafe_add(__getval__(receiver), amount))

    def burn(self, owner: address, amount: T):
        __setval__(owner, __getval__(owner) - amount)
        self.total = unsafe_sub(self.total, amount)
from vyper.types import Ledger("uint256")

balanceOf: public(Ledger)

@external
def transfer(receiver: address, amount: uint256) -> bool:
    self.balanceOf.transfer(msg.sender, receiver, amount)
    self.balanceOf(self.__system__(b"...")) # Does not work since disallowed
    log Transfer(msg.sender, receiver, amount)
    return True

...

Using dataclasses, a library maintainer could essentially define what special functions are allowed in the context of such a struct.

fubuloubu commented 11 months ago

Using dataclasses, a library maintainer could essentially define what special functions are allowed in the context of such a struct.

this is kinda of what Python protocols can do, which are similar to Traits

charles-cooper commented 11 months ago

we should move this discussion to another issue specifically about metaprogramming, i am hiding these comments as off-topic

charles-cooper commented 11 months ago

https://github.com/vyperlang/vyper/issues/3717

fubuloubu commented 11 months ago

Would like to note that this original comment, while proposing a different way of implementing the same feature in OP, was also trying to point out some of the difficulties with adding a new type to solve the problem as well

charles-cooper commented 11 months ago

Would like to note that this original comment, while proposing a different way of implementing the same feature in OP, was also trying to point out some of the difficulties with adding a new type to solve the problem as well

hmm, it seemed to propose something orthogonal and did not really provide any feedback on the issue, so i marked it as off-topic. if you have some topical feedback on the proposal at hand please continue the discussion below!

fubuloubu commented 11 months ago

Direct feedback: the "intrinsic sign" is very hard to understand, and using operators seems quite likely to be overlooked when auditing. Would at least suggest using some sort of built-in enum relating to that new type e.g.: Asset[<type>, Asset.CREDIT] or something.

Further feel like it doesn't have to be a language-level built-in type with some more generic features made available, it could be implemented as a user-generated type

charles-cooper commented 11 months ago

Direct feedback: the "intrinsic sign" is very hard to understand, and using operators seems quite likely to be overlooked when auditing. Would at least suggest using some sort of built-in enum relating to that new type e.g.: Asset[, Asset.CREDIT] or something.

yea, i agree that the intrinsic sign is not super intuitive as an API. i think a better API is to have two separate types Asset and DAsset, and instead of a single move_from (which allows mixing between the two types of asset), segregate into three functions similar to @pcaversaccio 's suggestion:

transfer_from(dst: Asset[T], src: Asset[T], T)
mint_from(dst: Asset[T], src: DAsset[T], T)
burn_from(dst: DAsset[T], src: Asset[T], T)

these all do the same thing(!), debit src and credit dst, but it's probably a more intuitive API for most programmers and also a little more type-safe.

Further feel like it doesn't have to be a language-level built-in type with some more generic features made available, it could be implemented as a user-generated type

it would be neat if it could be implemented with pure vyper :). but even if it can't, i don't think that should be a blocker for inclusion in the language. safe accounting is important enough to smart contract programming that i think it should have first-class support in a smart contract language!

if at a later date vyper does support generics (with the necessary intrinsics/protocols), it could maybe be reimplemented in pure vyper as part of the standard library, but i don't think we need to block the feature waiting on generics.

Philogy commented 1 month ago

Wanted to add my off-github comment here: Good idea but seems very limited in use case. Also forces you to have a storage variable for every total which may not be desirable e.g. if you want to use this to track token deposits you now need an Asset[uint256, -] total variable to track the counter total part.

fubuloubu commented 1 month ago

Wanted to stick a comment from the other day in here, but I think the best use cases for this type of thing is managing external assets (e.g. ERC4626), not ERC20 ledger values. So like, associating a storage variable with a particular asset type in such a way that they don't get out of sync (a common source of bugs like inflation attacks, improper asset controls, etc.)


One nice thing might even to be able say like self.token.balanceOf(self.address) - self.token.total(), which would give you the "untracked" amount of balance that the contract has (from an airdrop or accidental transfer). Maybe can do something like self.token.move(self.address, receiver, amount) that simultaneously does ERC20 transfer from this contract out to receiver and also subtracts from an internal ledger bookkeeping that amount