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

feat: update contract #7

Merged
merged 4 commits into from
Dec 13, 2024
Merged

Conversation

Tranduy1dol
Copy link
Member

@Tranduy1dol Tranduy1dol commented Aug 12, 2024

Check List

  • Don't forget to squash commits into meaningful chunks before merging
  • Check every test passed.
  • Did you split imports into std and custom parts?
  • Have you updated the consts in ORN?
  • Have you run ORN with the latest version?
  • Check your commit messages.
  • Have you added meaningful comments?
  • Don't forget to optimize gas cost!
    • Calling vector::length() multiple times is expensive (e.g. in loops), save it to a variable instead.
    • Consider using consts for things that can be pre-computed.
    • pow(2, x) => (1 << x+1)
    • inline short functions to save gas.
    • Find all vector::empty() in all files and replace them with the first append
    • Use vector::*_reverse_*() version to save gas.
  • Check Visibility of functions

@Tranduy1dol Tranduy1dol requested a review from zk-steve August 12, 2024 10:46
@Tranduy1dol Tranduy1dol force-pushed the mzk-42-settlement-contract-in-aptos branch from 421744b to bdbc6c2 Compare August 15, 2024 08:59
Move.toml Show resolved Hide resolved
sources/libs/bytes.move Outdated Show resolved Hide resolved
sources/libs/bytes.move Outdated Show resolved Hide resolved
@Tranduy1dol Tranduy1dol force-pushed the mzk-42-settlement-contract-in-aptos branch 8 times, most recently from b6913ed to afb5844 Compare August 22, 2024 03:09
Copy link
Contributor

Choose a reason for hiding this comment

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

remove has_registered_fact()
make init_fact_registry() as entry func
remove check if (exists(@starknet_addr) == false) {
init_fact_registry(signer);
};

Copy link
Contributor

Choose a reason for hiding this comment

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

remove if (verifier_fact.any_fact_registered == false) {
verifier_fact.any_fact_registered = true;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why register_fact() doesn't have any validation?

@Tranduy1dol Tranduy1dol force-pushed the mzk-42-settlement-contract-in-aptos branch from afb5844 to e7dbb49 Compare August 22, 2024 03:30
let main_public_input_length: u256 = (vector::length(&program_output) as u256);
let main_public_input_hash: vector<u8> = keccak256(bytes::vec_to_bytes_be(&program_output));
let main_public_input_length: u256 = (vector::length(program_output) as u256);
let main_public_input_hash: vector<u8> = keccak256(bytes::vec_to_bytes_be(program_output));
let buffer = vector::empty<u8>();
Copy link
Contributor

Choose a reason for hiding this comment

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

use main_public_input_hash as buffer directly

Copy link
Contributor

@zk-steve zk-steve Aug 22, 2024

Choose a reason for hiding this comment

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

find all vector::empty() in all files and replace them with the first append

@zk-steve zk-steve force-pushed the main branch 2 times, most recently from ff8a792 to 156884f Compare August 22, 2024 04:28
@@ -19,12 +14,11 @@ module starknet_addr::kzg_helper {
vector::length(commitment) == 48,
EINVALID_KZG_COMMITMENT
);
let versioned_hash_version_kzg = VERSIONED_HASH_VERSION_KZG;
let hash_commitment = hash::sha2_256(*commitment);
let hash_commitment_silce = vector::slice(&hash_commitment, 1, vector::length(&hash_commitment));
Copy link
Contributor

Choose a reason for hiding this comment

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

replace index 0 = VERSIONED_HASH_VERSION_KZG directly

Comment on lines 32 to 36
let versioned_hash = vector::slice(&bytes, 0, 32);
let z = vector::slice(&bytes, 32, 64);
let y = vector::slice(&bytes, 64, 96);
let commitment = vector::slice(&bytes, 96, 144);
let proof = vector::slice(&bytes, 144, 192);
Copy link
Contributor

Choose a reason for hiding this comment

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

    let proof = vector::trim(&mut bytes, 144);
    let commitment = vector::trim(&mut bytes, 96);
    let y = vector::trim_reverse(&mut bytes, 64);
    let z = vector::trim_reverse(&mut bytes, 32);
    let versioned_hash = bytes;

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this file

@Tranduy1dol Tranduy1dol force-pushed the mzk-42-settlement-contract-in-aptos branch 3 times, most recently from 3964fd3 to e4220be Compare August 22, 2024 06:45
let result = vector::empty<u8>();
vector::append(&mut result, versioned_hash_version_kzg);
vector::append(&mut result, hash_commitment_silce);
let fisrt_byte = vector::borrow_mut(&mut hash_commitment, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

vector::borrow_mut(&mut hash_commitment, 0) = VERSIONED_HASH_VERSION_KZG

global_root,
block_number,
block_hash
));
}

#[view]
Copy link
Contributor

@zk-steve zk-steve Aug 22, 2024

Choose a reason for hiding this comment

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

move all view functions down below the entry functions

Copy link
Contributor

Choose a reason for hiding this comment

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

public fun get_blob_hash(commitment: &vector<u8>): vector<u8> {

=> public inline fun get_blob_hash(commitment: &vector): vector {

@@ -140,7 +136,6 @@ module starknet_addr::starknet_validity {
onchain_data_hash: u256,
Copy link
Contributor

Choose a reason for hiding this comment

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

public fun state_block_number(): -> public inline fun state_block_number():

assert!(
state_block_number() == initial_block_number + 1,
EINVALID_FINAL_BLOCK_NUMBER
);
}

public fun update_state_internal(
public fun update_internal_state(
Copy link
Contributor

@zk-steve zk-steve Aug 22, 2024

Choose a reason for hiding this comment

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

Suggested change
public fun update_internal_state(
public(friend) fun update_internal_state(

Copy link
Contributor

Choose a reason for hiding this comment

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

public fun initialize(s: &signer, program_hash: u256, verifier: address, config_hash: u256, state: State)
should only allow to init if the resource hasn't been created

Copy link
Contributor

Choose a reason for hiding this comment

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

all set* func should be public(friend)

Copy link
Contributor

Choose a reason for hiding this comment

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

do it with all other files

@@ -230,7 +222,7 @@ module starknet_addr::starknet_validity {
verify_kzg_proof(&kzg_segment, &kzg_proof);

let state_transition_fact = keccak256(vec_to_bytes_be(&program_output));
update_state_internal(&program_output, state_transition_fact);
update_internal_state(&program_output, state_transition_fact);

// Re-entrancy protection: validate final block number
Copy link
Contributor

Choose a reason for hiding this comment

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

no Reentrancy in Move, remove this check, and the first line

@@ -230,7 +222,7 @@ module starknet_addr::starknet_validity {
verify_kzg_proof(&kzg_segment, &kzg_proof);
Copy link
Contributor

Choose a reason for hiding this comment

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

let kzg_segment = vector::slice(&pre_kzg_segment, 0u64, KZG_SEGMENT_SIZE);

=> let kzg_segment = vector::slice(&pre_kzg_segment, 0, KZG_SEGMENT_SIZE);

Copy link
Contributor

@zk-steve zk-steve Aug 22, 2024

Choose a reason for hiding this comment

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

let pre_kzg_segment = vector::slice(
&program_output,
HEADER_SIZE,
vector::length(&program_output)
);
use trim

@Tranduy1dol Tranduy1dol force-pushed the mzk-42-settlement-contract-in-aptos branch from e4220be to bd7fbaf Compare August 22, 2024 09:46
state,
sidecar
});
if (!is_initialized(verifier)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

assert(!is_initialized(verifier), ERR)

storage.program_hash = new_program_hash;
}

public fun get_program_hash(addr: address): u256 acquires Storage {
borrow_global_mut<Storage>(addr).program_hash
}

public fun set_config_hash(storage: &mut Storage, new_config_hash: u256) {
public(friend) fun set_config_hash(storage: &mut Storage, new_config_hash: u256) {
storage.config_hash = new_config_hash;
}

public fun get_config_hash(addr: address): u256 acquires Storage {
borrow_global_mut<Storage>(addr).config_hash
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
borrow_global_mut<Storage>(addr).config_hash
borrow_global<Storage>(addr).config_hash

Copy link
Contributor

Choose a reason for hiding this comment

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

apply for all similar errors above


let (precompile_output, ok) = pre_compile::point_evaluation_precompile(precompile_input);
assert!(ok, EPOINT_EVALUATION_PRECOMPILE_CALL_FAILED);
let precompile_output = pre_compile::point_evaluation_precompile(blob_hash);

assert!(
keccak256(precompile_output) == POINT_EVALUATION_PRECOMPILE_OUTPUT,
EUNEXPECTED_POINT_EVALUATION_PRECOMPILE_OUTPUT
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add TODO: check blob registration

@Tranduy1dol Tranduy1dol force-pushed the mzk-42-settlement-contract-in-aptos branch 2 times, most recently from 1eebaff to 5dcc3e1 Compare August 29, 2024 08:56
@Tranduy1dol Tranduy1dol force-pushed the mzk-42-settlement-contract-in-aptos branch from 5dcc3e1 to d477ed9 Compare September 12, 2024 02:44
@Draply Draply force-pushed the mzk-42-settlement-contract-in-aptos branch from d477ed9 to 3e6e119 Compare September 12, 2024 03:00
@Draply Draply force-pushed the mzk-42-settlement-contract-in-aptos branch from 198119d to b4b9244 Compare September 12, 2024 03:22
@Tranduy1dol Tranduy1dol merged commit d77f32b into main Dec 13, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants