Skip to content

rpc(ChainRpc): Add time_added_to_pool field for ChainRpcImpl::get_transaction#3949

Merged
zhangsoledad merged 4 commits intonervosnetwork:developfrom
eval-exec:exec/expose-TxPoolEntry.timestamp
Apr 25, 2023
Merged

rpc(ChainRpc): Add time_added_to_pool field for ChainRpcImpl::get_transaction#3949
zhangsoledad merged 4 commits intonervosnetwork:developfrom
eval-exec:exec/expose-TxPoolEntry.timestamp

Conversation

@eval-exec
Copy link
Collaborator

@eval-exec eval-exec commented Apr 21, 2023

What problem does this PR solve?

Issue Number: close #3938

Problem Summary:

What is changed and how it works?

What's Changed:

Related changes

  • Add time_added_to_pool field to TransactionWithStatus

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • None

Release note

Title Only: Include only the PR title in the release note.

@eval-exec eval-exec changed the title feat: dd enter_tx_pool_timestamp field to get_transaction feat: Add enter_tx_pool_timestamp field to get_transaction Apr 21, 2023
@eval-exec eval-exec changed the title feat: Add enter_tx_pool_timestamp field to get_transaction rpc(ChainRpc): Add enter_tx_pool_timestamp field for ChainRpcImpl::get_transaction Apr 21, 2023
@eval-exec
Copy link
Collaborator Author

eval-exec commented Apr 21, 2023

I think there may be a bug in the make gen-rpc-doc command.

Investigating...


Update: It seems that if I write multiple lines of comments to a field for a struct, make gen-rpc-doc may get unexpected result.

Waiting #3954 to solve rpc doc issue.

@eval-exec eval-exec force-pushed the exec/expose-TxPoolEntry.timestamp branch from a23d582 to 8c15c64 Compare April 21, 2023 09:18
@eval-exec eval-exec marked this pull request as ready for review April 21, 2023 09:41
@eval-exec eval-exec requested a review from a team as a code owner April 21, 2023 09:41
@eval-exec eval-exec requested review from doitian, quake and zhangsoledad and removed request for a team April 21, 2023 09:41
@eval-exec eval-exec self-assigned this Apr 21, 2023
@eval-exec eval-exec added the t:enhancement Type: Feature, refactoring. label Apr 21, 2023
@eval-exec eval-exec marked this pull request as draft April 21, 2023 10:52
@eval-exec eval-exec force-pushed the exec/expose-TxPoolEntry.timestamp branch 7 times, most recently from 71adf86 to 89d7fb9 Compare April 24, 2023 00:46
@eval-exec eval-exec marked this pull request as ready for review April 24, 2023 01:55
rpc/README.md Outdated

* `cycles`: [`Cycle`](#type-cycle) `|` `null` - The transaction consumed cycles.

* `enter_tx_pool_timestamp`: [`Uint64`](#type-uint64) `|` `null` - If the transaction is in tx-pool, `enter_tx_pool_timestamp` represent when it enter the tx-pool
Copy link
Member

Choose a reason for hiding this comment

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

I asked AI to give a better name:

Some better name options for enter_tx_pool_timestamp could be:

  • tx_pool_entry_timestamp
  • timestamp_entered_tx_pool
  • tx_pool_inclusion_time
  • time_added_to_tx_pool
    These names are a bit more verbose but convey the meaning clearly that it is the timestamp when the transaction entered the transaction pool.

I think we can use time_added_to_pool. The prefix tx_ is not necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. ❤️

@eval-exec eval-exec marked this pull request as draft April 24, 2023 03:51
Signed-off-by: Eval EXEC <execvy@gmail.com>
Signed-off-by: Eval EXEC <execvy@gmail.com>
@eval-exec eval-exec force-pushed the exec/expose-TxPoolEntry.timestamp branch from 89d7fb9 to d0f0ad1 Compare April 24, 2023 04:00
@eval-exec eval-exec marked this pull request as ready for review April 24, 2023 04:01
@eval-exec eval-exec requested a review from doitian April 24, 2023 04:05
@eval-exec eval-exec changed the title rpc(ChainRpc): Add enter_tx_pool_timestamp field for ChainRpcImpl::get_transaction rpc(ChainRpc): Add time_added_to_pool field for ChainRpcImpl::get_transaction Apr 24, 2023
@zhangsoledad zhangsoledad added this pull request to the merge queue Apr 24, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 24, 2023
@eval-exec eval-exec marked this pull request as draft April 24, 2023 14:50
@eval-exec
Copy link
Collaborator Author

eval-exec commented Apr 24, 2023

--- STDERR:              ckb-rpc tests::examples::test_rpc_examples ---
thread 'tests::examples::test_rpc_examples' panicked at 'assertion failed: `(left == right)`: RPC Example get_transaction(id=42) got the following unexpected response:

{"id":42,"jsonrpc":"2.0","result":{"cycles":"0x219","time_added_to_pool":"0x187b3d137a1","transaction":{"cell_deps":[{"dep_type":"code","out_point":{"index":"0x0","tx_hash":"0xa4037a893eb48e18ed4ef61034ce26eba9c585f15c9cee102ae58505565eccc3"}}],"hash":"0xa0ef4eb5f4ceeb08a4c8524d84c5da95dce2f608e0ca2ec8091191b0f330c6e3","header_deps":["0x7978ec7ce5b507cfb52e149e36b1a23f6062ed150503c85bbf825da3599095ed"],"inputs":[{"previous_output":{"index":"0x0","tx_hash":"0x365698b50ca0da75dca2c87f9e7b563811d3b5813736b8cc62cc3b106faceb17"},"since":"0x0"}],"outputs":[{"capacity":"0x2540be400","lock":{"args":"0x","code_hash":"0x28e83a1277d48add8e72fadaa9248559e1b632bab2bd60b27955ebc4c03800a5","hash_type":"data"},"type":null}],"outputs_data":["0x"],"version":"0x0","witnesses":[]},"tx_status":{"block_hash":null,"reason":null,"status":"pending"}},"error":null}

Diff < left / right > :
 RpcTestResponse {
     id: 42,
     jsonrpc: "2.0",
     result: Object {
         "cycles": String("0x219"),
>        "time_added_to_pool": String("0x187b3d137a1"),
         "transaction": Object {
             "cell_deps": Array [
                 Object {
                     "dep_type": String("code"),
                     "out_point": Object {
                         "index": String("0x0"),
                         "tx_hash": String("0xa4037a893eb48e18ed4ef61034ce26eba9c585f15c9cee102ae58505565eccc3"),
                     },
                 },
             ],
             "hash": String("0xa0ef4eb5f4ceeb08a4c8524d84c5da95dce2f608e0ca2ec8091191b0f330c6e3"),
             "header_deps": Array [
                 String("0x7978ec7ce5b507cfb52e149e36b1a23f6062ed150503c85bbf825da3599095ed"),
             ],
             "inputs": Array [
                 Object {
                     "previous_output": Object {
                         "index": String("0x0"),
                         "tx_hash": String("0x365698b50ca0da75dca2c87f9e7b563811d3b5813736b8cc62cc3b106faceb17"),
                     },
                     "since": String("0x0"),
                 },
             ],
             "outputs": Array [
                 Object {
                     "capacity": String("0x2540be400"),
                     "lock": Object {
                         "args": String("0x"),
                         "code_hash": String("0x28e83a1277d48add8e72fadaa9248559e1b632bab2bd60b27955ebc4c03800a5"),
                         "hash_type": String("data"),
                     },
                     "type": Null,
                 },
             ],
             "outputs_data": Array [
                 String("0x"),
             ],
             "version": String("0x0"),
             "witnesses": Array [],
         },
         "tx_status": Object {
             "block_hash": Null,
             "reason": Null,
             "status": String("pending"),
         },
     },
     error: Null,
 }

', rpc/src/tests/examples.rs:560:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

https://github.com/nervosnetwork/ckb/actions/runs/4786689804/jobs/8510943774

It's hard to let this RPC pass RpcTestExample, thinking...

Update: In latest commit, I reset Example json value of time_added_to_pool to RpcTestExample's response value. Then all of the unit tests passed.

I think this PR is ready for merge.

@eval-exec eval-exec marked this pull request as ready for review April 24, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t:enhancement Type: Feature, refactoring.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Show the timestamp of the transaction in tx-pool through rpc

3 participants