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

add pending txn subscription support #3885

Merged
merged 1 commit into from
Sep 29, 2021
Merged

add pending txn subscription support #3885

merged 1 commit into from
Sep 29, 2021

Conversation

rlan35
Copy link
Contributor

@rlan35 rlan35 commented Sep 28, 2021

Feed promoted pending txns into the pending txn subscription channel, so all subscribed source can get the new pending txns.

@rlan35 rlan35 requested a review from gupadhyaya September 28, 2021 22:50
@rlan35 rlan35 merged commit 4f973db into harmony-one:main Sep 29, 2021
@sophoah
Copy link
Contributor

sophoah commented Sep 30, 2021

@rlan35 was the PR been tested ? can you attached the test logs if you did ? Also since we have type change what is the impact of our/dapp RPC usage ? do we need to any special communication regarding that change ?

@gupadhyaya
Copy link
Contributor

@rlan35 was the PR been tested ? can you attached the test logs if you did ? Also since we have type change what is the impact of our/dapp RPC usage ? do we need to any special communication regarding that change ?

this should not have any regressions. the test logs are in the issue: #3881
should be ok to release.

@sophoah
Copy link
Contributor

sophoah commented Sep 30, 2021

see test done on testnet here : #3881 (comment)

@sophoah
Copy link
Contributor

sophoah commented Sep 30, 2021

@rlan35 not sure if normal but the test i did shows a duplicate tx each time.

node web3-Pendingtx-ws.js
0x769520008066520063131436e0eb9bdb
0xd085ab5e305a7f224aca5650f5ee0345d8a58e77729ea5205407cef15b7d6143
0xd085ab5e305a7f224aca5650f5ee0345d8a58e77729ea5205407cef15b7d6143
0x712d2f3f25692363726e352ffb377ba7592e2566cae89250afe4dd749524efa2
0x712d2f3f25692363726e352ffb377ba7592e2566cae89250afe4dd749524efa2

it's a minor issue but just wanted to let you know

@rlan35
Copy link
Contributor Author

rlan35 commented Sep 30, 2021

@rlan35 not sure if normal but the test i did shows a duplicate tx each time.

node web3-Pendingtx-ws.js
0x769520008066520063131436e0eb9bdb
0xd085ab5e305a7f224aca5650f5ee0345d8a58e77729ea5205407cef15b7d6143
0xd085ab5e305a7f224aca5650f5ee0345d8a58e77729ea5205407cef15b7d6143
0x712d2f3f25692363726e352ffb377ba7592e2566cae89250afe4dd749524efa2
0x712d2f3f25692363726e352ffb377ba7592e2566cae89250afe4dd749524efa2

it's a minor issue but just wanted to let you know

@gupadhyaya is this normal, did you notice this in your local testing?

@damiensgit
Copy link

damiensgit commented Sep 30, 2021

Just a note that whilst pending txes worked on my local build as above, it appears that getting the transaction for the hash always returns nil rather than the tx struct data (on go-etherum client) e.g.

tx, is_pending, _ := client.TransactionByHash(context.Background(), transactionHash)

is_pending is true but tx is always nil

On other evm chains, this works as expected, and a quick test in etherjs shows the same issues so it's not a go issue- eth_gettransaction rpc method isn't able to get the tx details for the hash even tho it's in the node's mempool it seems - is the a harmony limitation or a sep. bug?

@damiensgit
Copy link

damiensgit commented Sep 30, 2021

A web3 exmple of my earlier comment:

const web3 = new Web3("testnet"); 
var subscription = web3.eth.subscribe("pendingTransactions", function(err, result) {
    if (!err){
        console.log(result)
    }
}).on("data", function(transaction){
    console.log(transaction)
    web3.eth.getTransaction(transaction).then(console.log);
})


0xd085ab5e305a7f224aca5650f5ee0345d8a58e77729ea5205407cef15b7d6143
null

@orlandodiaz
Copy link

Im having the same issue as damien. I also get null transaction data on pending transactions. This only happens on my private rpc node and not on the hmy public one though

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.

5 participants