Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

refactor inner_call function for hashing syscalls #33067

Closed
wants to merge 0 commits into from
Closed

refactor inner_call function for hashing syscalls #33067

wants to merge 0 commits into from

Conversation

sanjsingh07
Copy link
Contributor

Problem

inner_call() function for hashing syscalls (sha256, keccak and blake3) are nearly identical

Summary of Changes

code refactor, ref #33046.

@mergify mergify bot added community Community contribution need:merge-assist labels Aug 30, 2023
@mergify mergify bot requested a review from a team August 30, 2023 13:42
@steviez steviez requested a review from t-nelson August 30, 2023 15:17
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

sorry but this is a no-op change. it retains all the replicode but moves it into another function and adds complexity with a selector enum.

the functions in question should be made generic over a trait

@sanjsingh07
Copy link
Contributor Author

Hi, Thanks for the reply,

yes, I tried using the generic approach like :

fn hash_fn<HasherType>(
    invoke_context: &mut InvokeContext,
    vals_addr: u64,
    vals_len: u64,
    result_addr: u64,
    hash_bytes: u64,
    hash_func: &mut HasherType,
    memory_mapping: &mut MemoryMapping,
) -> Result<u64, Error>
where
    HasherType: Hasher,
{
    // Remaining implementation
}

but I couldn't figure out how to tackle Hasher as it was struct, and didn't know if I needed to create a new custom Hasher trait that Implements by (sha256, keccak, and blake3) for example:

// Define a trait for hashers
trait HasherTrait {
    fn hash(&mut self, bytes: &[u8]);
}

// Implement HasherTrait for solana_sdk::hash::Hasher
impl HasherTrait for Hasher {
    fn hash(&mut self, bytes: &[u8]) {
        self.update(bytes);
    }
}
``` and same for (keccak and blake3).

if that's the case I might need a little help with it, but in the meantime, I'll keep looking and see if I can tackle it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community Community contribution need:merge-assist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants