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

Add deployer preset #467

Merged
merged 23 commits into from
Oct 21, 2022
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions src/openzeppelin/utils/presets/UniversalDeployer.cairo
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts for Cairo v0.4.0 (utils/presets/UniversalDeployer.cairo)

%lang starknet
martriay marked this conversation as resolved.
Show resolved Hide resolved

from starkware.starknet.common.syscalls import get_caller_address, deploy
from starkware.cairo.common.cairo_builtins import HashBuiltin
from starkware.cairo.common.hash import hash2
from starkware.cairo.common.bool import FALSE, TRUE

@event
func ContractDeployed(
address: felt,
deployer: felt,
unique: felt,
classHash: felt,
calldata_len: felt,
calldata: felt*,
salt: felt
martriay marked this conversation as resolved.
Show resolved Hide resolved
) {
}

@external
func deployContract{
syscall_ptr: felt*,
pedersen_ptr: HashBuiltin*,
range_check_ptr
}(
classHash: felt,
salt: felt,
unique: felt,
calldata_len: felt,
calldata: felt*
) -> (address: felt) {
alloc_locals;
let (deployer) = get_caller_address();

local _salt;
local from_zero;
if (unique == TRUE) {
let (unique_salt) = hash2{hash_ptr=pedersen_ptr}(deployer, salt);
_salt = unique_salt;
from_zero = FALSE;
tempvar _pedersen = pedersen_ptr;
} else {
_salt = salt;
from_zero = TRUE;
tempvar _pedersen = pedersen_ptr;
}

tempvar pedersen_ptr = _pedersen;
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need to increment ap here with tempvar when a reference is enough:

Suggested change
tempvar pedersen_ptr = _pedersen;
let pedersen_ptr = _pedersen;

Actually, the reference shouldn't even be necessary, from my understanding, if you rename _perdersen to pedersen_ptr rebinding the reference (because in both branches we should have [ap - 1]). But, for some reason, when doing this, the line emitting the event seems to revoke the reference. This is an expected behavior @bbrandtom ?

Copy link
Member

Choose a reason for hiding this comment

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

I mean, this works (as I expect):

    if (unique == TRUE) {
        let (unique_salt) = hash2{hash_ptr=pedersen_ptr}(deployer, salt);
        _salt = unique_salt;
        from_zero = FALSE;
        tempvar pedersen_ptr = pedersen_ptr;
    } else {
        _salt = salt;
        from_zero = TRUE;
        tempvar pedersen_ptr = pedersen_ptr;
    }

    let (address) = deploy(
        class_hash=classHash,
        contract_address_salt=_salt,
        constructor_calldata_size=calldata_len,
        constructor_calldata=calldata,
        deploy_from_zero=from_zero,
    );

    return (address=address)

While this doesn't (I would expect it to work too):

    if (unique == TRUE) {
        let (unique_salt) = hash2{hash_ptr=pedersen_ptr}(deployer, salt);
        _salt = unique_salt;
        from_zero = FALSE;
        tempvar pedersen_ptr = pedersen_ptr;
    } else {
        _salt = salt;
        from_zero = TRUE;
        tempvar pedersen_ptr = pedersen_ptr;
    }

    let (address) = deploy(
        class_hash=classHash,
        contract_address_salt=_salt,
        constructor_calldata_size=calldata_len,
        constructor_calldata=calldata,
        deploy_from_zero=from_zero,
    );


    ContractDeployed.emit(
        address=address,
        deployer=deployer,
        unique=unique,
        classHash=classHash,
        calldata_len=calldata_len,
        calldata=calldata,
        salt=salt
    );

    return (address=address)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ericnordelo what do you mean by it doesn't work? what exact error are you getting? and i'm not sure the point you're trying to make by removing the event emission

Copy link
Member

Choose a reason for hiding this comment

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

The point is that the event emission is revoking the pedersen_ptr reference in the example I'm presenting when I think it shouldn't. And when you rebind the reference to the same ap reference, it works again (with the event). This looks like a potential compiler issue.

Choose a reason for hiding this comment

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

@ericnordelo What's the rebinding that works with the event? My guess is that the revocation happens due to an arbitrary change in ap caused by a function call. Usually (after 0.5.0 IIRC), such revocations are solved implicitly by adding a local variable definition prior to the call. However, here the function itself is generated by the compiler (emit is replaced by a code using the emit_event syscall) so it might be missed.

Copy link
Member

@ericnordelo ericnordelo Oct 19, 2022

Choose a reason for hiding this comment

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

Hi @ArielElp, the rebinding that works with the event is the one after the else block in this example:

    (...)
    if (unique == TRUE) {
        let (unique_salt) = hash2{hash_ptr=pedersen_ptr}(deployer, salt);
        _salt = unique_salt;
        from_zero = FALSE;
        tempvar pedersen_ptr = pedersen_ptr;
    } else {
        _salt = salt;
        from_zero = TRUE;
        tempvar pedersen_ptr = pedersen_ptr;
    }
    
    // This is the rebinding:
    let pedersen_ptr = pedersen_ptr;

    let (address) = deploy(
        class_hash=classHash,
        contract_address_salt=_salt,
        constructor_calldata_size=calldata_len,
        constructor_calldata=calldata,
        deploy_from_zero=from_zero,
    );

    ContractDeployed.emit(
        address=address,
        deployer=deployer,
        unique=unique,
        classHash=classHash,
        calldata_len=calldata_len,
        calldata=calldata,
        salt=salt
    );

    return (address=address)

If you remove that rebinding, the reference is revoked by the emit call. My question is, why is it not being revoked when we just rebind the reference from ap to itself? (the same reference to the exact ap location).

That rebinding to itself is avoiding the reference revocation in the emit call, and I think it should work without that line too, but is being revoked without it, what am I missing?


let (address) = deploy(
class_hash=classHash,
contract_address_salt=_salt,
constructor_calldata_size=calldata_len,
constructor_calldata=calldata,
deploy_from_zero=from_zero,
);

ContractDeployed.emit(
address=address,
deployer=deployer,
unique=unique,
classHash=classHash,
calldata_len=calldata_len,
calldata=calldata,
martriay marked this conversation as resolved.
Show resolved Hide resolved
salt=salt
martriay marked this conversation as resolved.
Show resolved Hide resolved
);

return (address=address);
}
12 changes: 6 additions & 6 deletions tests/account/test_Account.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
import pytest
from signers import MockSigner, get_raw_invoke
from utils import assert_revert, get_contract_class, cached_contract, TRUE, State, Account
from utils import assert_revert, get_contract_class, cached_contract, TRUE, IACCOUNT_ID, State, Account


signer = MockSigner(123456789987654321)
other = MockSigner(987654321123456789)

IACCOUNT_ID = 0xa66bd575


@pytest.fixture(scope='module')
def contract_classes():
Expand Down Expand Up @@ -112,7 +110,7 @@ async def test_return_value(account_factory):
read_info = await signer.send_transactions(account, [(initializable.contract_address, 'initialized', [])])
call_info = await initializable.initialized().call()
(call_result, ) = call_info.result
assert read_info.call_info.retdata[1] == call_result #1
assert read_info.call_info.retdata[1] == call_result # 1


@ pytest.mark.asyncio
Expand All @@ -138,7 +136,8 @@ async def test_nonce(account_factory):

# higher nonce
await assert_revert(
signer.send_transactions(account, [(initializable.contract_address, 'initialize', [])], nonce=current_nonce + 1),
signer.send_transactions(account, [(
initializable.contract_address, 'initialize', [])], nonce=current_nonce + 1),
reverted_with="Invalid transaction nonce. Expected: {}, got: {}.".format(
current_nonce, current_nonce + 1
)
Expand Down Expand Up @@ -184,7 +183,8 @@ async def test_account_takeover_with_reentrant_call(account_factory):
account, _, _, _, attacker = account_factory

await assert_revert(
signer.send_transaction(account, attacker.contract_address, 'account_takeover', []),
signer.send_transaction(
account, attacker.contract_address, 'account_takeover', []),
reverted_with="Account: no reentrant call"
)

Expand Down
4 changes: 1 addition & 3 deletions tests/account/test_EthAccount.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
import pytest
from utils import assert_revert, get_contract_class, cached_contract, TRUE, FALSE, State
from utils import assert_revert, get_contract_class, cached_contract, TRUE, FALSE, IACCOUNT_ID, State
from signers import MockEthSigner, get_raw_invoke

private_key = b'\x01' * 32
signer = MockEthSigner(b'\x01' * 32)
other = MockEthSigner(b'\x02' * 32)

IACCOUNT_ID = 0xa66bd575


@pytest.fixture(scope='module')
def contract_defs():
Expand Down
2 changes: 1 addition & 1 deletion tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
ZERO_ADDRESS = 0
TRUE = 1
FALSE = 0

IACCOUNT_ID = 0xa66bd575
martriay marked this conversation as resolved.
Show resolved Hide resolved

_root = Path(__file__).parent.parent

Expand Down
102 changes: 102 additions & 0 deletions tests/utils/test_UniversalDeployer.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import pytest
from starkware.starknet.core.os.contract_address.contract_address import calculate_contract_address_from_hash
from starkware.crypto.signature.fast_pedersen_hash import pedersen_hash
from starkware.starknet.core.os.class_hash import compute_class_hash

from signers import MockSigner
from utils import (
State,
Account,
get_contract_class,
assert_event_emitted,
cached_contract,
IACCOUNT_ID,
FALSE,
TRUE,
)

signer = MockSigner(123456789987654321)


@pytest.fixture(scope='module')
def contract_classes():
account_cls = Account.get_class
deployer_cls = get_contract_class('UniversalDeployer')

return account_cls, deployer_cls


@pytest.fixture(scope='module')
async def deployer_init(contract_classes):
_, deployer_cls = contract_classes
starknet = await State.init()
account = await Account.deploy(signer.public_key)
deployer = await starknet.deploy(contract_class=deployer_cls)
return (
starknet.state,
account,
deployer
)


@pytest.fixture
def deployer_factory(contract_classes, deployer_init):
account_cls, deployer_cls = contract_classes
state, account, deployer = deployer_init
_state = state.copy()
_account = cached_contract(_state, account_cls, account)
deployer = cached_contract(_state, deployer_cls, deployer)

return _account, deployer


@pytest.mark.asyncio
@pytest.mark.parametrize('unique', [TRUE, FALSE])
async def test_deployment(deployer_factory, unique):
account, deployer = deployer_factory
salt = 1234567875432 # random value
calldata = [signer.public_key]
class_hash = compute_class_hash(
contract_class=Account.get_class, hash_func=pedersen_hash)

# deploy contract
params = [class_hash, salt, unique, len(calldata), *calldata]
deploy_exec_info = await signer.send_transaction(account, deployer.contract_address, 'deployContract', params)
deployed_address = deploy_exec_info.call_info.retdata[1]

# check address
if unique:
actual_salt = pedersen_hash(account.contract_address, salt)
deployer_address = deployer.contract_address
else:
actual_salt = salt
deployer_address = 0

expected_address = calculate_contract_address_from_hash(
salt=actual_salt,
class_hash=class_hash,
constructor_calldata=calldata,
deployer_address=deployer_address
)

assert deployed_address == expected_address

# check deployment
tx_exec_info = await signer.send_transaction(account, deployed_address, 'supportsInterface', [IACCOUNT_ID])
is_account = tx_exec_info.call_info.retdata[1]
assert is_account == TRUE

assert_event_emitted(
deploy_exec_info,
from_address=deployer.contract_address,
name='ContractDeployed',
data=[
deployed_address, # contractAddress
account.contract_address, # deployer
unique, # unique
class_hash, # classHash
len(calldata), # calldata_len
*calldata, # calldata
salt, # salt
]
)