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

refactor: transactions #3502

Merged
11 commits merged into from Nov 14, 2022
Merged

refactor: transactions #3502

11 commits merged into from Nov 14, 2022

Conversation

ghost
Copy link

@ghost ghost commented Nov 7, 2022

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

Open API Spec Version Changes (if applicable)

SwarmCommon.yaml: Minor : 2.3.0

Motivation and Context (Optional)

Related Issue (Optional)

#3468
closes #3505

Screenshots (if appropriate):


This change is Reviewable

@ghost ghost requested review from ralph-pichler, metacertain and istae and removed request for ralph-pichler November 7, 2022 08:31
@ghost ghost self-assigned this Nov 7, 2022
@ghost ghost marked this pull request as ready for review November 7, 2022 11:25
pkg/api/transaction_test.go Outdated Show resolved Hide resolved
Copy link
Member

@istae istae left a comment

Choose a reason for hiding this comment

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

nice work, but some changes requested

@ghost ghost changed the base branch from resolved-london-tx to master November 8, 2022 23:09
@ghost ghost force-pushed the resolved-signer-eip branch from 9af73d7 to 33c9f2a Compare November 9, 2022 00:50
@ghost ghost requested a review from istae November 9, 2022 00:51
Copy link
Member

@istae istae left a comment

Choose a reason for hiding this comment

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

missing gas fee and tip and in CancelTrx method

@istae
Copy link
Member

istae commented Nov 9, 2022

Also, we can debate if this PR is necessary. Maybe we do not care about storing gas fee and tip?

@ldeffenb
Copy link
Collaborator

ldeffenb commented Nov 9, 2022

Also, we can debate if this PR is necessary. Maybe we do not care about storing gas fee and tip?

It would be nice to know these values for a transaction that ends up hung for some reason. By storing them, then the /transactions API can recall them to assist in debugging.

@ghost ghost requested a review from istae November 10, 2022 06:04
Copy link
Member

@istae istae left a comment

Choose a reason for hiding this comment

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

please update api version before merging

pkg/api/transaction.go Outdated Show resolved Hide resolved
@ghost ghost requested a review from istae November 10, 2022 16:05
pkg/transaction/transaction.go Outdated Show resolved Hide resolved
@ghost ghost requested a review from istae November 10, 2022 17:25
pkg/api/transaction.go Outdated Show resolved Hide resolved
@ghost ghost requested a review from istae November 14, 2022 09:31
@ghost ghost merged commit cc1f281 into master Nov 14, 2022
@ghost ghost deleted the resolved-signer-eip branch November 14, 2022 12:36
@aloknerurkar aloknerurkar added this to the 1.10.0 milestone Dec 8, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants