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.

Sign up for GitHub

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

Jump to bottom

refactor: move from transaction id to transaction hash #479

Merged
merged 50 commits into from
Sep 18, 2024
Merged

refactor: move from transaction id to transaction hash #479

merged 50 commits into from
Sep 18, 2024

Conversation

denishacquin
Copy link
Contributor

@denishacquin denishacquin commented Sep 6, 2024 •
edited
Loading

Fixes #451

What

Move frome id to hash in the transaction model.

  • Changed the model and renamed methods, endpoints, etc
  • Created a migration to add hash column in the db (for now keeping the id, but we can remove it when we feel comfortable)
  • Created a method to generate hashes similarly to eth in transaction_processor
  • Had to add nonces for hash uniqueness (for now it's on-the-fly on the backend side, I have a branch with the whole endpoint and frontend nonce management but that could be done in SIM-Add nonce to transactions #468 )
image image

Why

To get closer to eth spec.

Testing done

  • unit tests passed
  • tested migration
  • tested feature
  • E2E OK

Checks

  • I have tested this code
  • I have reviewed my own PR
  • I have created an issue for this PR
  • I have set a descriptive PR title compliant with conventional commits

Reviewing tips

  • Need to particularly pay attention to the generate hash method and the DB migration

User facing release notes

  • Implemented transaction hashes

@denishacquin denishacquin changed the title 451 refactor transaction id with transaction hash refactor: move from transaction id to transaction hash Sep 6, 2024
@denishacquin denishacquin self-assigned this Sep 6, 2024
@denishacquin denishacquin linked an issue Sep 6, 2024 that may be closed by this pull request
Refactor transaction id with transaction Hash #451
Closed
@denishacquin denishacquin mentioned this pull request Sep 6, 2024
Closed
Den and others added 11 commits September 6, 2024 13:56
cristiam86
cristiam86 requested changes Sep 13, 2024
View reviewed changes
Copy link
Collaborator

@cristiam86 cristiam86 left a comment

Choose a reason for hiding this comment

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

Very good job, only one endpoint that got restored when it was removed before

backend/protocol_rpc/endpoints.py Outdated Show resolved Hide resolved
AgustinRamiroDiaz
AgustinRamiroDiaz approved these changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@AgustinRamiroDiaz AgustinRamiroDiaz left a comment

Choose a reason for hiding this comment

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

Changes look good! all minor comments

My main question is the one about the transaction hash being the current ID

backend/database_handler/migration/versions/99d3f1bc5a08_add_hash.py Outdated Show resolved Hide resolved
backend/database_handler/migration/versions/99d3f1bc5a08_add_hash.py Outdated Show resolved Hide resolved
backend/database_handler/models.py Outdated Show resolved Hide resolved
backend/database_handler/models.py Outdated Show resolved Hide resolved
backend/database_handler/transactions_processor.py Outdated Show resolved Hide resolved
frontend/src/hooks/useContractQueries.ts Outdated Show resolved Hide resolved
frontend/src/hooks/useDb.ts Outdated Show resolved Hide resolved
@denishacquin denishacquin mentioned this pull request Sep 17, 2024
Open
@denishacquin denishacquin merged commit 9399c14 into main Sep 18, 2024
14 checks passed
@github-actions GitHub Actions
Copy link
Contributor

🎉 This PR is included in version 0.8.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@cristiam86 cristiam86 deleted the 451-refactor-transaction-id-with-transaction-hash branch October 16, 2024 15:08
@cristiam86 cristiam86 mentioned this pull request Oct 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AgustinRamiroDiaz AgustinRamiroDiaz AgustinRamiroDiaz approved these changes

@cristiam86 cristiam86 cristiam86 approved these changes

Assignees

@denishacquin denishacquin

Labels
released run-tests
Projects
None yet
Milestone
Align RPC With Proper Definition
Development

Successfully merging this pull request may close these issues.

Refactor transaction id with transaction Hash
3 participants
@denishacquin @cristiam86 @AgustinRamiroDiaz