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

[BLADE-97] Refactor tx signer #113

Merged
merged 4 commits into from
Feb 14, 2024
Merged

[BLADE-97] Refactor tx signer #113

merged 4 commits into from
Feb 14, 2024

Conversation

grujicf
Copy link

@grujicf grujicf commented Feb 12, 2024

Description

Refactored tx singer logic. If everything is fine, calcTxHash function should be deleted from txsigner.go. Also, files such as txsigner_london_berlin.go and other test files are no longer needed. It is necessary to write new tests.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Breaking changes

Please complete this section if any breaking changes have been made, otherwise delete it

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Copy link

github-actions bot commented Feb 12, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Copy link

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

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

Generally, let's follow the convention that file names are separated with the underscore, for the sake of the consistency. So:

  • txsignerEIP155.go -> txsigner_eip155.go
  • txsignerFrontier -> txsigner_frontier.go etc.

@Stefan-Ethernal
Copy link

Stefan-Ethernal commented Feb 13, 2024

@grujicf you need to post the following comment to pass the CLA assistant check:

I have read the CLA Document and I hereby sign the CLA

Also, there are some build errors and merge conflicts on the txsigner.go, so fix those before we continue reviewing the PR.

// RLP(chainId, nonce, gasPrice, gas, to, -, -, -)
hashPreimage.Set(RLP.NewNull())
} else {

Choose a reason for hiding this comment

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

Suggested change


// Checking whether the transaction is a smart contract deployment
if tx.To() == nil {

Choose a reason for hiding this comment

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

Suggested change

accessList := RLP.NewArray()

if tx.AccessList() != nil {

Choose a reason for hiding this comment

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

Suggested change


// accessTuple contains (address, storageKeys[])
for _, accessTuple := range tx.AccessList() {

Choose a reason for hiding this comment

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

Suggested change

@@ -87,7 +87,8 @@ func TestLondonSignerSender(t *testing.T) {
}

chainID := tc.chainID.Uint64()
signer := NewLondonOrBerlinSigner(chainID, true, NewEIP155Signer(chainID, true))
//signer := NewLondonOrBerlinSigner(chainID, true, NewEIP155Signer(chainID, true))
signer := NewLondonOrBerlinSigner(chainID, true, NewEIP155Signer(chainID))

Choose a reason for hiding this comment

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

We should remove NewLondonOrBerlinSigner.

Also there are some build errors, because crypto.NewEIP155Signer does not have bool parameter anymore, so please take a look to it.

@grujicf
Copy link
Author

grujicf commented Feb 13, 2024

I have read the CLA Document and I hereby sign the CLA

@Stefan-Ethernal
Copy link

@grujicf you have some merge conflicts on txsigner.go and go.sum, so pay attention to resolve it, because CI is blocked until there are outstanding conflicts. If you need any help with it, ping me.

@Stefan-Ethernal Stefan-Ethernal changed the base branch from feature/berlin to feat/refactor-tx-signers February 14, 2024 09:38
@Stefan-Ethernal Stefan-Ethernal merged commit 2999e85 into Ethernal-Tech:feat/refactor-tx-signers Feb 14, 2024
3 of 7 checks passed
Stefan-Ethernal pushed a commit that referenced this pull request Feb 19, 2024
* Refactoring the tx singer logic

* Build fix

* Fix tests
goran-ethernal pushed a commit that referenced this pull request Feb 23, 2024
* Refactoring the tx singer logic

* Build fix

* Fix tests
Stefan-Ethernal added a commit that referenced this pull request Feb 26, 2024
* [BLADE-97] Refactor tx signer (#113)

* Refactoring the tx singer logic

* Build fix

* Fix tests

* Revert go.mod and go.sum

* Linter fixes and renames

* Fix tests

* Fix test

* Fix tests

* Fix EIP155Signer condition

* Fix

* Fix

* Fix

* Fix

* Remove calcTxHash

* Fix lint

* Revert go.mod

* Address comments (part 1)

* Rebase fix

* code reorg

* Lint fix

* Lint fix

* err logging

* Remove commented code

* ChainID check in tx signer

* Lint fix

* Fix legacy tests

* UTs fix

* Fix TestTxPool_RecoverableError

* Change error message in the Sender fn

* Minor change

---------

Co-authored-by: grujicf <61869071+grujicf@users.noreply.github.com>
Co-authored-by: grujicf998 <grujicf998@gmail.com>
Co-authored-by: Goran Rojovic <goran.rojovic@ethernal.tech>
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.

2 participants