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

Library_packed #336

Open
ponderingdemocritus opened this issue May 23, 2022 · 16 comments
Open

Library_packed #336

ponderingdemocritus opened this issue May 23, 2022 · 16 comments

Comments

@ponderingdemocritus
Copy link

🧐 Motivation
The current design pattern for all ERC tokens is to use the uint256 as this interfaces with EVM. This is understandable. However, not all use cases of these tokens need a pure uint256 standard. The most important part is that we keep the interface of the uint256 but in the library.cairo we can make a low-level conversion into a felt if it is appropriate for the use case. It is then the responsibility of the developer to choose what they wish to use.

For the upcoming ERC1155 standard this is particularly helpful as it will effectively half the storage space for the tokens.

📝 Details
Proposes design pattern:

library_packed.cairo

This file would simply have a low-level conversion from uint to felt and vice versa where needed. This would then just be chosen by the developer when they deploy the contract.

Thoughts?

@Pet3ris
Copy link

Pet3ris commented May 23, 2022

@ponderingdemocritus have been thinking along similar lines (using uint256 as interface and handling internal compute with felt) and had people in the hackathon ask how to do the conversion - very supportive of this proposal.

@ponderingdemocritus
Copy link
Author

ponderingdemocritus commented May 23, 2022

Here is a rough implementation. Thanks Lancelot...

%lang starknet

from starkware.starknet.common.syscalls import get_caller_address
from starkware.cairo.common.cairo_builtins import HashBuiltin
from starkware.cairo.common.math import assert_not_zero, assert_not_equal, assert_lt_felt
from starkware.cairo.common.alloc import alloc
from starkware.cairo.common.uint256 import (
    Uint256, uint256_add, uint256_sub, uint256_le, uint256_check)
from starkware.cairo.common.math import split_felt
from openzeppelin.introspection.IERC165 import IERC165
from openzeppelin.token.erc1155.interfaces.IERC1155_Receiver import IERC1155_Receiver

const IERC1155_interface_id = 0xd9b67a26
const IERC1155_MetadataURI_interface_id = 0x0e89341c
const IERC165_interface_id = 0x01ffc9a7

const IERC1155_RECEIVER_ID = 0x4e2312e0
const ON_ERC1155_RECEIVED_SELECTOR = 0xf23a6e61
const ON_BATCH_ERC1155_RECEIVED_SELECTOR = 0xbc197c81
const IACCOUNT_ID = 0xf10dbd44

#
# Events
#

@event
func TransferSingle(operator : felt, from_ : felt, to : felt, id : Uint256, value : Uint256):
end

@event
func TransferBatch(
        operator : felt, from_ : felt, to : felt, ids_len : felt, ids : Uint256*, values_len : felt,
        values : Uint256*):
end

@event
func ApprovalForAll(account : felt, operator : felt, approved : felt):
end

@event
func URI(value_len : felt, value : felt*, id : Uint256):
end

#
# Storage
#

@storage_var
func ERC1155_balances_(id : Uint256, account : felt) -> (balance : felt):
end

@storage_var
func ERC1155_operator_approvals_(account : felt, operator : felt) -> (approved : felt):
end

# TODO: decide URI format
@storage_var
func ERC1155_uri_() -> (uri : felt):
end

#
# Constructor
#

func ERC1155_initializer{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        uri_ : felt):
    _setURI(uri_)
    return ()
end

#
# Getters
#

func ERC1155_supportsInterface(interface_id : felt) -> (res : felt):
    # Less expensive (presumably) than storage
    if interface_id == IERC1155_interface_id:
        return (1)
    end
    if interface_id == IERC1155_MetadataURI_interface_id:
        return (1)
    end
    if interface_id == IERC165_interface_id:
        return (1)
    end
    return (0)
end

func ERC1155_uri{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}() -> (
        uri : felt):
    let (uri) = ERC1155_uri_.read()
    return (uri)
end

func ERC1155_balanceOf{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        account : felt, id : Uint256) -> (balance : Uint256):
    with_attr error_message("ERC1155: balance query for the zero address"):
        assert_not_zero(account)
    end
    let (balance) = ERC1155_balances_.read(id=id, account=account)
    let (balance_felt) = _felt_to_uint(balance)
    return (balance_felt)
end

func ERC1155_balanceOfBatch{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        accounts_len : felt, accounts : felt*, ids_len : felt, ids : Uint256*) -> (
        batch_balances_len : felt, batch_balances : Uint256*):
    alloc_locals
    # Check args are equal length arrays
    with_attr error_message("ERC1155: accounts and ids length mismatch"):
        assert ids_len = accounts_len
    end
    # Allocate memory
    let (local batch_balances : Uint256*) = alloc()
    let len = accounts_len
    # Call iterator
    balance_of_batch_iter(len, accounts, ids, batch_balances)
    return (batch_balances_len=len, batch_balances=batch_balances)
end

func ERC1155_isApprovedForAll{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        account : felt, operator : felt) -> (approved : felt):
    let (approved) = ERC1155_operator_approvals_.read(account=account, operator=operator)
    return (approved)
end

#
# Externals
#

func ERC1155_setApprovalForAll{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        operator : felt, approved : felt):
    let (caller) = get_caller_address()
    # Non-zero caller asserted in called function
    _set_approval_for_all(owner=caller, operator=operator, approved=approved)
    return ()
end

func ERC1155_safeTransferFrom{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        from_ : felt, to : felt, id : Uint256, amount : Uint256):
    let (caller) = get_caller_address()
    with_attr error_message("ERC1155: called from zero address"):
        assert_not_zero(caller)
    end
    with_attr error_message("ERC1155: caller is not owner nor approved"):
        owner_or_approved(from_)
    end
    _safe_transfer_from(from_, to, id, amount)
    return ()
end

func ERC1155_safeBatchTransferFrom{
        syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        from_ : felt, to : felt, ids_len : felt, ids : Uint256*, amounts_len : felt,
        amounts : Uint256*):
    let (caller) = get_caller_address()
    with_attr error_message("ERC1155: called from zero address"):
        assert_not_zero(caller)
    end
    with_attr error_message("ERC1155: transfer caller is not owner nor approved"):
        owner_or_approved(from_)
    end
    return _safe_batch_transfer_from(from_, to, ids_len, ids, amounts_len, amounts)
end

#
# Internals
#

func _safe_transfer_from{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        from_ : felt, to : felt, id : Uint256, amount : Uint256):
    alloc_locals
    # Check args
    with_attr error_message("ERC1155: transfer to the zero address"):
        assert_not_zero(to)
    end
    with_attr error_message("ERC1155: invalid uint in calldata"):
        uint256_check(id)
        uint256_check(amount)
    end
    # Todo: beforeTokenTransfer

    # Check balance sufficient
    let (local from_balance) = ERC1155_balances_.read(id=id, account=from_)
    let (balance_uint) = _felt_to_uint(from_balance)
    let (sufficient_balance) = uint256_le(amount, balance_uint)
    with_attr error_message("ERC1155: insufficient balance for transfer"):
        assert_not_zero(sufficient_balance)
    end
    # Deduct from sender
    let (new_balance : Uint256) = uint256_sub(balance_uint, amount)

    let (new_balance_uint) = _uint_to_felt(new_balance)
    ERC1155_balances_.write(id=id, account=from_, value=new_balance_uint)

    # Add to reciever
    let (to_balance) = ERC1155_balances_.read(id=id, account=to)
    let (to_balance_uint) = _felt_to_uint(from_balance)
    let (new_balance : Uint256, carry) = uint256_add(to_balance_uint, amount)
    let (new_balance_uint) = _uint_to_felt(new_balance)
    with_attr error_message("arithmetic overflow"):
        assert carry = 0
    end
    ERC1155_balances_.write(id=id, account=to, value=new_balance_uint)

    let (operator) = get_caller_address()

    TransferSingle.emit(operator, from_, to, id, amount)

    _do_safe_transfer_acceptance_check(operator, from_, to, id, amount)

    return ()
end

func _safe_batch_transfer_from{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        from_ : felt, to : felt, ids_len : felt, ids : Uint256*, amounts_len : felt,
        amounts : Uint256*):
    alloc_locals
    with_attr error_message("ERC1155: ids and amounts length mismatch"):
        assert_not_zero(to)
    end
    # Check args are equal length arrays
    with_attr error_message("ERC1155: transfer to the zero address"):
        assert ids_len = amounts_len
    end
    # Recursive call
    let len = ids_len
    safe_batch_transfer_from_iter(from_, to, len, ids, amounts)
    let (operator) = get_caller_address()
    TransferBatch.emit(operator, from_, to, ids_len, ids, amounts_len, amounts)
    _do_safe_batch_transfer_acceptance_check(
        operator, from_, to, ids_len, ids, amounts_len, amounts)
    return ()
end

func ERC1155_mint{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        to : felt, id : Uint256, amount : Uint256):
    let (caller) = get_caller_address()
    with_attr error_message("ERC1155: called from zero address"):
        assert_not_zero(caller)
    end
    # Cannot mint to zero address
    with_attr error_message("ERC1155: mint to the zero address"):
        assert_not_zero(to)
    end
    # Check uints valid
    with_attr error_message("ERC1155: invalid uint256 in calldata"):
        uint256_check(id)
        uint256_check(amount)
    end
    # beforeTokenTransfer
    # add to minter check for overflow
    let (to_balance) = ERC1155_balances_.read(id=id, account=to)
    let (to_balance_uint) = _felt_to_uint(to_balance)
    let (new_balance : Uint256, carry) = uint256_add(to_balance_uint, amount)
    with_attr error_message("ERC1155: arithmetic overflow"):
        assert carry = 0
    end
    let (new_balance_uint) = _uint_to_felt(new_balance)
    ERC1155_balances_.write(id=id, account=to, value=new_balance_uint)
    # doSafeTransferAcceptanceCheck
    let (operator) = get_caller_address()
    TransferSingle.emit(operator=operator, from_=0, to=to, id=id, value=amount)
    _do_safe_transfer_acceptance_check(operator, 0, to, id, amount)

    return ()
end

func ERC1155_mint_batch{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        to : felt, ids_len : felt, ids : Uint256*, amounts_len : felt, amounts : Uint256*):
    alloc_locals
    let (caller) = get_caller_address()
    with_attr error_message("ERC1155: called from zero address"):
        assert_not_zero(caller)
    end
    # Cannot mint to zero address
    with_attr error_message("ERC1155: mint to the zero address"):
        assert_not_zero(to)
    end
    # Check args are equal length arrays
    with_attr error_message("ERC1155: ids and amounts length mismatch"):
        assert ids_len = amounts_len
    end
    # Recursive call
    let len = ids_len
    mint_batch_iter(to, len, ids, amounts)
    let (operator) = get_caller_address()
    TransferBatch.emit(
        operator=operator,
        from_=0,
        to=to,
        ids_len=ids_len,
        ids=ids,
        values_len=amounts_len,
        values=amounts)
    _do_safe_batch_transfer_acceptance_check(operator, 0, to, ids_len, ids, amounts_len, amounts)
    return ()
end

func ERC1155_burn{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        from_ : felt, id : Uint256, amount : Uint256):
    alloc_locals
    let (caller) = get_caller_address()
    with_attr error_message("ERC1155: called from zero address"):
        assert_not_zero(caller)
    end
    with_attr error_message("ERC1155: burn from the zero address"):
        assert_not_zero(from_)
    end
    # beforeTokenTransfer
    # Check balance sufficient
    let (local from_balance) = ERC1155_balances_.read(id=id, account=from_)
    let (to_from_balance_uint) = _felt_to_uint(from_balance)
    let (sufficient_balance) = uint256_le(amount, to_from_balance_uint)
    with_attr error_message("ERC1155: burn amount exceeds balance"):
        assert_not_zero(sufficient_balance)
    end
    # Deduct from burner
    let (new_balance) = uint256_sub(to_from_balance_uint, amount)
    let (new_balance_uint) = _uint_to_felt(new_balance)
    ERC1155_balances_.write(id=id, account=from_, value=new_balance_uint)
    let (operator) = get_caller_address()
    TransferSingle.emit(operator=operator, from_=from_, to=0, id=id, value=amount)
    return ()
end

func ERC1155_burn_batch{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        from_ : felt, ids_len : felt, ids : Uint256*, amounts_len : felt, amounts : Uint256*):
    alloc_locals
    let (caller) = get_caller_address()
    with_attr error_message("ERC1155: called from zero address"):
        assert_not_zero(caller)
    end
    with_attr error_message("ERC1155: burn from the zero address"):
        assert_not_zero(from_)
    end
    # Check args are equal length arrays
    with_attr error_message("ERC1155: ids and amounts length mismatch"):
        assert ids_len = amounts_len
    end
    # Recursive call
    let len = ids_len
    burn_batch_iter(from_, len, ids, amounts)
    let (operator) = get_caller_address()
    TransferBatch.emit(
        operator=operator,
        from_=from_,
        to=0,
        ids_len=ids_len,
        ids=ids,
        values_len=amounts_len,
        values=amounts)
    return ()
end

#
# Internals
#

func _set_approval_for_all{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        owner : felt, operator : felt, approved : felt):
    # check approved is bool
    assert approved * (approved - 1) = 0
    # since caller can now be 0
    with_attr error_message("ERC1155: setting approval status for zero address"):
        assert_not_zero(owner * operator)
    end
    with_attr error_message("ERC1155: setting approval status for self"):
        assert_not_equal(owner, operator)
    end
    ERC1155_operator_approvals_.write(owner, operator, approved)
    ApprovalForAll.emit(owner, operator, approved)
    return ()
end

func _setURI{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(newuri : felt):
    ERC1155_uri_.write(newuri)
    return ()
end

func _do_safe_transfer_acceptance_check{
        syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        operator : felt, from_ : felt, to : felt, id : Uint256, amount : Uint256):
    let (caller) = get_caller_address()
    # ERC1155_RECEIVER_ID = 0x4e2312e0
    let (is_supported) = IERC165.supportsInterface(to, IERC1155_RECEIVER_ID)
    if is_supported == 1:
        let (selector) = IERC1155_Receiver.onERC1155Received(to, operator, from_, id, amount)

        # onERC1155Recieved selector
        with_attr error_message("ERC1155: ERC1155Receiver rejected tokens"):
            assert selector = ON_ERC1155_RECEIVED_SELECTOR
        end
        return ()
    end
    let (is_account) = IERC165.supportsInterface(to, IACCOUNT_ID)
    with_attr error_message("ERC1155: transfer to non ERC1155Receiver implementer"):
        assert_not_zero(is_account)
    end
    # IAccount_ID = 0x50b70dcb
    return ()
end

func _do_safe_batch_transfer_acceptance_check{
        syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        operator : felt, from_ : felt, to : felt, ids_len : felt, ids : Uint256*,
        amounts_len : felt, amounts : Uint256*):
    let (caller) = get_caller_address()
    # Confirm supports IERC1155Reciever interface
    let (is_supported) = IERC165.supportsInterface(to, IERC1155_RECEIVER_ID)
    if is_supported == 1:
        let (selector) = IERC1155_Receiver.onERC1155BatchReceived(
            to, operator, from_, ids_len, ids, amounts_len, amounts)

        # Confirm onBatchERC1155Recieved selector returned
        with_attr error_message("ERC1155: ERC1155Receiver rejected tokens"):
            assert selector = ON_BATCH_ERC1155_RECEIVED_SELECTOR
        end
        return ()
    end

    # Alternatively confirm EOA
    let (is_account) = IERC165.supportsInterface(to, IACCOUNT_ID)
    with_attr error_message("ERC1155: transfer to non ERC1155Receiver implementer"):
        assert_not_zero(is_account)
    end
    return ()
end

#
# Helpers
#

func balance_of_batch_iter{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        len : felt, accounts : felt*, ids : Uint256*, batch_balances : Uint256*):
    if len == 0:
        return ()
    end
    # may be unnecessary now
    # Read current entries, Todo: perform Uint256 checks
    let id : Uint256 = [ids]
    uint256_check(id)
    let account : felt = [accounts]

    let (balance : Uint256) = ERC1155_balanceOf(account, id)
    assert [batch_balances] = balance
    return balance_of_batch_iter(
        len - 1, accounts + 1, ids + Uint256.SIZE, batch_balances + Uint256.SIZE)
end

func safe_batch_transfer_from_iter{
        syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        from_ : felt, to : felt, len : felt, ids : Uint256*, amounts : Uint256*):
    # Base case
    alloc_locals
    if len == 0:
        return ()
    end

    # Read current entries,  perform Uint256 checks
    let id = [ids]
    with_attr error_message("ERC1155: invalid uint in calldata"):
        uint256_check(id)
    end
    let amount = [amounts]
    with_attr error_message("ERC1155: invalid uint in calldata"):
        uint256_check(amount)
    end

    # Check balance is sufficient
    let (from_balance) = ERC1155_balances_.read(id=id, account=from_)
    let (from_balance_uint) = _felt_to_uint(from_balance)
    let (sufficient_balance) = uint256_le(amount, from_balance_uint)
    with_attr error_message("ERC1155: insufficient balance for transfer"):
        assert_not_zero(sufficient_balance)
    end
    # deduct from
    let (new_balance : Uint256) = uint256_sub(from_balance_uint, amount)
    let (from_new_balance_felt) = _uint_to_felt(new_balance)    
    ERC1155_balances_.write(id=id, account=from_, value=from_new_balance_felt)

    # add to
    let (to_balance) = ERC1155_balances_.read(id=id, account=to)
    let (to_balance_uint) = _felt_to_uint(to_balance)
    let (new_balance : Uint256, carry) = uint256_add(to_balance_uint, amount)
    with_attr error_message("arithmetic overflow"):
        assert carry = 0  # overflow protection
    end
    let (from_new_balance_felt) = _uint_to_felt(new_balance) 
    ERC1155_balances_.write(id=id, account=to, value=from_new_balance_felt)

    # Recursive call
    return safe_batch_transfer_from_iter(
        from_, to, len - 1, ids + Uint256.SIZE, amounts + Uint256.SIZE)
end

func mint_batch_iter{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        to : felt, len : felt, ids : Uint256*, amounts : Uint256*):
    # Base case
    alloc_locals
    if len == 0:
        return ()
    end

    # Read current entries, Todo: perform Uint256 checks
    let id : Uint256 = [ids]
    let amount : Uint256 = [amounts]
    with_attr error_message("ERC1155: invalid uint256 in calldata"):
        uint256_check(id)
        uint256_check(amount)
    end
    # add to
    let (to_balance) = ERC1155_balances_.read(id=id, account=to)
    let (to_balance_uint) = _felt_to_uint(to_balance)
    let (new_balance : Uint256, carry) = uint256_add(to_balance_uint, amount)
    with_attr error_message("ERC1155: arithmetic overflow"):
        assert carry = 0  # overflow protection
    end
    let (new_balance_felt) = _uint_to_felt(new_balance) 
    ERC1155_balances_.write(id=id, account=to, value=new_balance_felt)

    # Recursive call
    return mint_batch_iter(to, len - 1, ids + Uint256.SIZE, amounts + Uint256.SIZE)
end

func burn_batch_iter{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(
        from_ : felt, len : felt, ids : Uint256*, amounts : Uint256*):
    # Base case
    alloc_locals
    if len == 0:
        return ()
    end

    # Read current entries, Todo: perform Uint256 checks
    let id : Uint256 = [ids]
    with_attr error_message("ERC1155: invalid uint in calldata"):
        uint256_check(id)
    end
    let amount : Uint256 = [amounts]
    with_attr error_message("ERC1155: invalid uint in calldata"):
        uint256_check(amount)
    end

    # Check balance is sufficient
    let (from_balance) = ERC1155_balances_.read(id=id, account=from_)
    let (from_balance_uint) = _felt_to_uint(from_balance)
    let (sufficient_balance) = uint256_le(amount, from_balance_uint)
    with_attr error_message("ERC1155: burn amount exceeds balance"):
        assert_not_zero(sufficient_balance)
    end

    # deduct from
    let (new_balance : Uint256) = uint256_sub(from_balance_uint, amount)

    let (new_balance_felt) = _uint_to_felt(new_balance)

    ERC1155_balances_.write(id=id, account=from_, value=new_balance_felt)

    # Recursive call
    return burn_batch_iter(from_, len - 1, ids + Uint256.SIZE, amounts + Uint256.SIZE)
end

func owner_or_approved{syscall_ptr : felt*, pedersen_ptr : HashBuiltin*, range_check_ptr}(owner):
    let (caller) = get_caller_address()
    if caller == owner:
        return ()
    end
    let (approved) = ERC1155_isApprovedForAll(owner, caller)
    assert approved = 1
    return ()
end

func _uint_to_felt{
        syscall_ptr: felt*,
        pedersen_ptr: HashBuiltin*,
        range_check_ptr
    } (value: Uint256) -> (value: felt):
    assert_lt_felt(value.high, 2**123)
    return (value.high * (2 ** 128) + value.low)
end

func _felt_to_uint{
        syscall_ptr: felt*,
        pedersen_ptr: HashBuiltin*,
        range_check_ptr
    } (value: felt) -> (value: Uint256):
    let (high, low) = split_felt(value)
    tempvar res: Uint256
    res.high = high
    res.low = low
    return (res)
end

@pscott
Copy link
Contributor

pscott commented May 23, 2022

regarding _uint_to_felt: uint256 lib provides a uint256_check. I think simply checking for value.high is not enough, because what happens if value.low > 2**128 ?

@ponderingdemocritus
Copy link
Author

good point - it would be better to use that check. I think this pattern could be extended out to 721 and 20 if people request it. The 721 could be crucial if users are bulk transferring as the cost will be halved.

@EvolveArt
Copy link
Contributor

Looks like a good idea, curious to see if in practice it really halves the cost tho. Is it worth the overhead ? Probably, yes.
Curious as well of what are the use cases where you don't think uint256 is required ? I see a lot of issues that could potentially arise from this if developers are not cautious - still supportive of this optimization tho.

@ponderingdemocritus
Copy link
Author

ponderingdemocritus commented May 23, 2022

well, it is two memory stores instead of 1. So for example in Realms we have 22 resources and an AMM. So in the current implementation, this is 44 memory slots to write on an AMM trade. Halving this is huge.

I am trying to work out exact edge cases to arise from this and would love to know more. In .sol there is an 1155 packed balance contract that stores balances in bitmaps. But you have a max limit on how many tokens you can store if you choose max compression. But this is the developer's choice to make at deploy time according to their use case.

@Th0rgal
Copy link

Th0rgal commented May 24, 2022

Hi, the proposal seems interesting but I'm not sure I understand the interest from a storage point of view. If a uint256 is composed of two felts, as long as one felt is not exceeded, the other one remains initialized to zero. I'm not sure about the implementation of fees right now, but in principle we shouldn't have to pay storage fees as long as we don't change the felt value, should we?

@pscott
Copy link
Contributor

pscott commented May 24, 2022

@Th0rgal the way I understand it is:

  • A felt can be up to 2^251 bits (size of P).
  • A uint256 can be up to 2^256
  • The Uint256 (struct) is not "take the first 251 bits of uint and store it in .low, then take the 5 remaining bits and store it in .high; .low and .high correspond to the 128 lowest/highest bits.
  • So a number like 2^129, which is NOT bigger than a felt, will not simply use one storage, but two storages (2^128 in .low and 2 in .high)

I might be wrong here, but if I'm right, then you can see that indeed this proposal would optimise the storage for any number that is bigger than 2^128 :)

@Th0rgal
Copy link

Th0rgal commented May 24, 2022

Sorry I may not have correctly understood. If I'm not mistaken, we only have to pay for the state changes, not the storage itself. In your example the amount of bits to update doesn't change, does it?

@ponderingdemocritus
Copy link
Author

yes, very valid point. I think you might be correct, the only question is if storage structs costs more to update, or if they count towards two storage updates on every write

@pscott
Copy link
Contributor

pscott commented May 24, 2022

I don't think I follow here:

  1. Take the number 2^129
  2. If we store it using a Uin256 struct then we will do two storage writes (.low and .high) (two different storage slots)
  3. If we store it using a single felt we will do one storage write (a single storage slot)

So to me it looks like 3. is better. Am I missing something? :)

@ponderingdemocritus
Copy link
Author

The question is now - if the 0 element within the uint256 never actually gets updated from 0, then it shouldn't actually cost anything extra than this implementation as the actual state change only happens on 1 memory cell anyway.

@pscott
Copy link
Contributor

pscott commented May 24, 2022

Yes but from what I understand, for any number bigger or equal to 2^129, the second element (.high) will get updated.
Since a felt is 2^251, it is indeed possible that the second element (.high field) will get updated :)

@ponderingdemocritus
Copy link
Author

Yes, correct. So then this whole optimization becomes moot if the use case never gets above 2^129. So it's not worth the low-level change at that point, because the use case was planning on using a number less than 2^129 anyway...

@pscott
Copy link
Contributor

pscott commented May 25, 2022

Yes, correct. So then this whole optimization becomes moot if the use case never gets above 2^129. So it's not worth the low-level change at that point, because the use case was planning on using a number less than 2^129 anyway...

Oh ok I thought the goal for this optim was to store up to a felt, not necessarily something smaller than 2^129 :)

@milancermak
Copy link
Contributor

Using felts instead of Uint256 would also save on transaction execution resources.

I was curious about the savings. I forked the repo and created a library_felt for ERC20 (code can be found in this branch). The public API is kept the same, but internally, it uses felts for everything.

In a simple transfer TX went from needing 870 steps and 32 range_check_builtins to 683 steps and 18 range_check_builtins; the rest stayed the same. That's a saving of about 15 gas / transfer (870 - 683) * 0.05 + (32 - 18) * 0.4.

Not much in absolute terms, but somewhat significant in relative terms. Given that these contracts are going to be the canonical implementation of tokens used on Starknet, that we're still early and that scaled out to hundreds of millions of TX in the future, the savings are serious. I'd urge OZ to consider a felt-based implementation for ERC tokens as originally suggested.

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

No branches or pull requests

6 participants