Skip to content

fix: check mempool when call eth_getTransactionByHash#1526

Merged
Flouse merged 2 commits intomainfrom
protocol/yangby/bugfix/tx-in-mempool
Nov 13, 2023
Merged

fix: check mempool when call eth_getTransactionByHash#1526
Flouse merged 2 commits intomainfrom
protocol/yangby/bugfix/tx-in-mempool

Conversation

@chaoticlonghair
Copy link
Contributor

@chaoticlonghair chaoticlonghair commented Nov 6, 2023 •
edited
Loading

What this PR does / why we need it?

This PR resolves #1523 partially.

This PR requires #1530.

p.s. I say "partially" because no tests are updated or added.

What is the impact of this PR?

No Breaking Change

CI Settings

CI Usage

Tip: Check the CI you want to run below, and then comment /run-ci.

CI Switch

  • Web3 Compatible Tests
  • OCT 1-5 And 12-15
  • OCT 6-10
  • OCT 11
  • OCT 16-19
  • v3 Core Tests

CI Description

CI Name Description
Web3 Compatible Test Test the Web3 compatibility of Axon
v3 Core Test Run the compatibility tests provided by Uniswap V3
OCT 1-5 | 6-10 | 11 | 12-15 | 16-19 Run the compatibility tests provided by OpenZeppelin

@chaoticlonghair chaoticlonghair requested a review from a team as a code owner November 6, 2023 08:11
@github-actions github-actions bot added the bugfix label Nov 6, 2023
@chaoticlonghair

This comment was marked as off-topic.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@chaoticlonghair chaoticlonghair requested review from Flouse, KaoImin and driftluo and removed request for Simon-Tl and wenyuanhust November 6, 2023 08:17
@Flouse Flouse requested a review from sunchengzhu November 6, 2023 08:17
driftluo
driftluo previously approved these changes Nov 6, 2023
View reviewed changes
Flouse
Flouse previously approved these changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@Flouse Flouse left a comment

Choose a reason for hiding this comment

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

p.s. I say "partially" because no tests are updated or added.

You said that you used hardhat and Foundry to test #1523.
So, this PR is manually tested, right?

@chaoticlonghair
Copy link
Contributor Author

chaoticlonghair commented Nov 6, 2023

So, this PR is manually tested, right?

Yep!

KaoImin
KaoImin previously approved these changes Nov 6, 2023
View reviewed changes
@chaoticlonghair

This comment was marked as resolved.

Sign in to view
@chaoticlonghair chaoticlonghair mentioned this pull request Nov 6, 2023
Closed
sunchengzhu
sunchengzhu previously approved these changes Nov 6, 2023
View reviewed changes
@chaoticlonghair chaoticlonghair mentioned this pull request Nov 7, 2023
Merged
6 tasks
@sunchengzhu

This comment was marked as off-topic.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@chaoticlonghair chaoticlonghair force-pushed the protocol/yangby/bugfix/tx-in-mempool branch from b1f085d to fb1ad92 Compare November 8, 2023 06:24
@chaoticlonghair

This comment was marked as off-topic.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
chaoticlonghair added a commit to chaoticlonghair/axon-test that referenced this pull request Nov 8, 2023
@chaoticlonghair
fix: wait for tx committed before read its data (ref: axonweb3/axon#1526
57c4c44 
)
@chaoticlonghair chaoticlonghair mentioned this pull request Nov 8, 2023
Merged
Flouse pushed a commit to axonweb3/axon-test that referenced this pull request Nov 8, 2023
@chaoticlonghair
fix: wait for tx committed before read its data (ref: axonweb3/axon#1526
ac66a29 
) (#1)
@Flouse

This comment was marked as off-topic.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@chaoticlonghair chaoticlonghair mentioned this pull request Nov 8, 2023
Closed
@chaoticlonghair chaoticlonghair force-pushed the protocol/yangby/bugfix/tx-in-mempool branch from fb1ad92 to bc7336a Compare November 10, 2023 03:28
@chaoticlonghair chaoticlonghair mentioned this pull request Nov 10, 2023
Merged
@Flouse

This comment was marked as outdated.

Sign in to view
@Flouse Flouse enabled auto-merge November 10, 2023 10:20
@github-actions

This comment was marked as outdated.

Sign in to view
@chaoticlonghair chaoticlonghair changed the title fix: check mempool when call eth_getTransactionByHash ✋ [Hold] fix: check mempool when call eth_getTransactionByHash Nov 10, 2023
@chaoticlonghair

This comment was marked as outdated.

Sign in to view
@Flouse Flouse added this pull request to the merge queue Nov 10, 2023
@Flouse Flouse removed this pull request from the merge queue due to a manual request Nov 10, 2023
@Flouse

This comment was marked as outdated.

Sign in to view
@chaoticlonghair
Copy link
Contributor Author

chaoticlonghair commented Nov 12, 2023

Could NOT be passed since #1544.

Without the bugfix of #1544, all tests should be patched: waiting for all previous transactions mined before sending a new transaction.

@chaoticlonghair chaoticlonghair force-pushed the protocol/yangby/bugfix/tx-in-mempool branch from bc7336a to b180b27 Compare November 13, 2023 06:37
@chaoticlonghair

This comment was marked as off-topic.

Sign in to view
@chaoticlonghair chaoticlonghair changed the title ✋ [Hold] fix: check mempool when call eth_getTransactionByHash fix: check mempool when call eth_getTransactionByHash Nov 13, 2023
@github-actions
Copy link

github-actions bot commented Nov 13, 2023 •
edited by chaoticlonghair
Loading

CI tests run on commit:

CI test list:

Please check ci test results later.

@chaoticlonghair
Copy link
Contributor Author

chaoticlonghair commented Nov 13, 2023

@Flouse Do not do squash merging.
Keep the two commits to be separated.
The 2nd commit is a temporary solution and it could be reverted after the #1544 fixed.

Flouse
Flouse approved these changes Nov 13, 2023
View reviewed changes
@Flouse Flouse merged commit 4c3be3d into main Nov 13, 2023
@chaoticlonghair chaoticlonghair deleted the protocol/yangby/bugfix/tx-in-mempool branch November 13, 2023 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Flouse Flouse Flouse approved these changes

@driftluo driftluo driftluo left review comments

@sunchengzhu sunchengzhu Awaiting requested review from sunchengzhu

+1 more reviewer

@KaoImin KaoImin KaoImin left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bugfix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Send raw transaction successfully but failed to get it by its hash through web JSON-RPC apis.

5 participants

@chaoticlonghair @sunchengzhu @Flouse @driftluo @KaoImin