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

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

Merged

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
@@ -6936,6 +6936,8 @@ The JSON view of a transaction as well as its status.

* `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
@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