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: add nonce to txHash calculation #66

Merged
merged 4 commits into from
Jan 19, 2022
Merged

feat: add nonce to txHash calculation #66

merged 4 commits into from
Jan 19, 2022

Conversation

dan-ziv
Copy link
Collaborator

@dan-ziv dan-ziv commented Jan 18, 2022

Description of the Changes

Please add a detailed description of the change, whether it's an enhancement or a bugfix.
If the PR is related to an open issue please link to it.

Checklist

  • Changes have been done against master branch, and PR does not conflict
  • New unit / functional tests have been added (whenever applicable)
  • Test are passing in local environment
  • Docs have been updated
  • PR title is follow the standard-version convention: <type>(optional subject): <description>, e.g: fix: minor typos in code

This change is Reviewable

@dan-ziv dan-ziv self-assigned this Jan 18, 2022
@dan-ziv dan-ziv linked an issue Jan 18, 2022 that may be closed by this pull request
@dan-ziv dan-ziv added this to the alpha milestone Jan 18, 2022
@dan-ziv dan-ziv changed the title feat: nonce feat: add nonce to txHash calculation Jan 19, 2022
@dan-ziv dan-ziv requested a review from liorgold2 January 19, 2022 09:15
Copy link
Collaborator

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1, 1 of 6 files at r2.
Reviewable status: 2 of 8 files reviewed, 2 unresolved discussions (waiting on @dan-ziv and @liorgold2)


src/constants.js, line 5 at r2 (raw file):

export const LOCAL_STORAGE_TRANSFERS_KEY = 'STARKGATE_TRANSFERS';
export const L2_INVOKE_TX_PREFIX = '115923154332517';

do you need it to compute the hash of regular transactions?

Code quote:

export const L2_INVOKE_TX_PREFIX = '115923154332517'

src/hooks/useEventListener.js, line 37 at r2 (raw file):

      logger.log('Event received', {event});
      const {to_address, from_address, selector, payload, nonce} = event.returnValues;
      return txHash(from_address, to_address, selector, payload, chainId, nonce);

Thats a bad name if it works only for l1_handler.

Either pass a tx_hash_prefix param
or rename to L1HandlerTxHash

Code quote:

txHash

@dan-ziv
Copy link
Collaborator Author

dan-ziv commented Jan 19, 2022


src/hooks/useEventListener.js, line 37 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

Thats a bad name if it works only for l1_handler.

Either pass a tx_hash_prefix param
or rename to L1HandlerTxHash

That's should be generic.
I will soon generlize it to pass txHashPrefix but it will be in a separate change.

@dan-ziv
Copy link
Collaborator Author

dan-ziv commented Jan 19, 2022


src/constants.js, line 5 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

do you need it to compute the hash of regular transactions?

I'm not sure I need it, currently, I'm calc the txHash only from messages that sent to L2.

Copy link
Collaborator

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1, 4 of 6 files at r2, all commit messages.
Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @dan-ziv, @ilyalesokhin-starkware, and @liorgold2)


src/hooks/useEventListener.js, line 37 at r2 (raw file):

Previously, dan-ziv (Dan Ziv) wrote…

That's should be generic.
I will soon generlize it to pass txHashPrefix but it will be in a separate change.

I don't like it the replay to my other comment seems to indicate that you are going to support only l1_handlers but you still call this function txHash

@dan-ziv
Copy link
Collaborator Author

dan-ziv commented Jan 19, 2022


src/hooks/useEventListener.js, line 37 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

I don't like it the replay to my other comment seems to indicate that you are going to support only l1_handlers but you still call this function txHash

done

Copy link
Collaborator

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 6 files at r2, 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-ziv and @liorgold2)


src/hooks/useEventListener.js, line 37 at r2 (raw file):

Previously, dan-ziv (Dan Ziv) wrote…

done

thanks.

@dan-ziv dan-ziv merged commit 7f1441a into dev Jan 19, 2022
@dan-ziv dan-ziv deleted the feat/nonce branch January 19, 2022 13:09
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.

Handle nonce for calculating transaction hash
2 participants