Skip to content
This repository has been archived by the owner on Mar 12, 2020. It is now read-only.

fix: Fix transaction offset #1472

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

howjmay
Copy link
Member

@howjmay howjmay commented Jan 13, 2020

<>
In transaction_serialize_to_flex_trits() the value of
macro NUM_TRITS_* and NUM_FLEX_TRITS_* may not
always be the same values, so we need to unify them.

Test Plan:

<<Outline how the reviewer can verify & test your changes here>>

In `transaction_serialize_to_flex_trits()` the value of
macro `NUM_TRITS_*` and `NUM_FLEX_TRITS_*` may not
always be the same values, so we need to unify them.
@thibault-martinez
Copy link
Member

Hi @howjmay, the documentation of this function says that the size should be in trits, not flex_trits.

/// Inserts the contents of an array into another array starting at a given
/// index.
/// @param[in] to_flex_trits - the array to insert into
/// @param[in] to_len - the number of trits encoded in the to_flex_trits array
/// @param[in] flex_trits - the array containing the trits to copy over
/// @param[in] len - the number of trits the flex_trits array stores
/// @param[in] start - the start index in the destination array
/// @param[in] num_trits - the number of trits to copy over
/// @return size_t - the number of trits copied over
size_t flex_trits_insert(flex_trit_t *const to_flex_trits, size_t const to_len, flex_trit_t const *const flex_trits,
                         size_t const len, size_t const start, size_t const num_trits);

@howjmay
Copy link
Member Author

howjmay commented Jan 13, 2020

However I wonder that if the sizes used here are in trits, then how could the size used in line 133 is NUM_FLEX_TRITS_SERIALIZED_TRANSACTION. Doesn't it cause bufferover flow?

@thibault-martinez
Copy link
Member

This line 133 https://github.com/iotaledger/entangled/pull/1472/files#diff-fcfdd8b4cc72f7c24ae51e4061656b3aR133 ?

It looks good to me, it's only flex_trits_insert that needs size in trits.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants