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

restore jsonrpc api consistency to display only TRX as value #5469

Closed
wants to merge 1 commit into from

Conversation

alexqrid
Copy link

@alexqrid alexqrid commented Sep 4, 2023

What does this PR do?
This PR restores consistency in JSON-RPC API. Previously value field in JSON-RPC response displayed various currencies(TRC-10 or TRX) and this is incorrect.

Why are these changes required?
As we can't distinguish the transaction type in JSON-RPC response it's weird to return various currencies amount in value field or vote amount.
For instance, for transaction types UnfreezeAssetContract,TransferAssetContract value field indicates token amount, for VoteWitnessContract it indicates vote count, for Exchange*Contract it indicates unknown currency at all as the currency depends on exchange.

This PR has been tested by:

  • Manual Testing

Follow up

System information

java-tron version: GreatVoyage-v4.7.2
OS & Version: Linux

Expected behaviour

In order to keep consistent API, value field in JSON-RPC response should indicate TRX(TRON) amount, neither asset amount nor vote count.

Actual behaviour

value field indicates not only TRX(TRON) value but TRC-10 token value as well.

Check for example transaction 64e594ccb2cb47b23ade85adbcb3404a501d2627291b09a0ed49364049c5abfa:

eth_getTransactionByHash response
{
    "jsonrpc": "2.0",
    "id": 1,
    "result": {
        "blockHash": "0x0000000003197e6a40297fe291280f9c7ad3cc970564b93f96ced133d305a55e",
        "blockNumber": "0x3197e6a",
        "from": "0x4da5ddaaea7f1138c030ac8d97017d5020dd6536",
        "gas": "0x0",
        "gasPrice": "0x1a4",
        "hash": "0x64e594ccb2cb47b23ade85adbcb3404a501d2627291b09a0ed49364049c5abfa",
        "input": "0x",
        "nonce": "0x0000000000000000",
        "r": "0x49a16c9c202a9ab7b2b71e172e440d475733169a90514dc9fb6c89be576a57ad",
        "s": "0x150276b20b7aa21f96afbde95d4f31b30a10b0839bb8f7af85c99e274de248d3",
        "to": "0x49d216cb7b1cc41f166be76241301781f87e039a",
        "transactionIndex": "0x5",
        "type": "0x0",
        "v": "0x1c",
        "value": "0x87a238"
    }
}

Steps to reproduce the behaviour

curl --location 'http://node/jsonrpc' \
--header 'Content-Type: application/json' \
--data '{
    "id": 1,
    "jsonrpc": "2.0",
    "method": "eth_getTransactionByHash",
    "params": [
        "0x64e594ccb2cb47b23ade85adbcb3404a501d2627291b09a0ed49364049c5abfa"
    ]
}

Extra details
With this PR jsonrpc response is consistent and returns correct value of tron for the transacion above (0x64e594ccb2cb47b23ade85adbcb3404a501d2627291b09a0ed49364049c5abfa):

{
    "jsonrpc": "2.0",
    "id": 7704110,
    "result": {
        "blockHash": "0x0000000003197e6a40297fe291280f9c7ad3cc970564b93f96ced133d305a55e",
        "blockNumber": "0x3197e6a",
        "from": "0x4da5ddaaea7f1138c030ac8d97017d5020dd6536",
        "gas": "0x0",
        "gasPrice": "0x1a4",
        "hash": "0x64e594ccb2cb47b23ade85adbcb3404a501d2627291b09a0ed49364049c5abfa",
        "input": "0x",
        "nonce": "0x0000000000000000",
        "r": "0x49a16c9c202a9ab7b2b71e172e440d475733169a90514dc9fb6c89be576a57ad",
        "s": "0x150276b20b7aa21f96afbde95d4f31b30a10b0839bb8f7af85c99e274de248d3",
        "to": "0x49d216cb7b1cc41f166be76241301781f87e039a",
        "transactionIndex": "0x5",
        "type": "0x0",
        "v": "0x1c",
        "value": "0x0"
    }
}

@317787106
Copy link
Contributor

I don't agree with this modification. It's not compatible with the data ago. And tronscan uses this value though it's not TRX. I think it's not necessary to do it.

@alexqrid
Copy link
Author

alexqrid commented Sep 6, 2023

And tronscan uses this value though it's not TRX. I think it's not necessary to do it.

Thank you for the answer. Let me clarify some points:
Actually nobody can use it without querying /wallet/gettransaction*//wallet/gettransactioninfo*, as they need to identify the type of transaction(interacted system contract in the transaction), but all needed values with more identifiable currency information are located in the response of the mentioned endpoints, but not in jsornpc.
If tronscan uses it, it just make an extra api call to the node to retrieve data that can be parsed from previous responses.
If we make the transaction type identifiable in JSON-RPC response, then one can use it as you mentioned, but at the moment it's just an extra API call.

@halibobo1205
Copy link
Contributor

@317787106 Use type to make the difference? ,see 83a6d58. What is type used for, I don't see it being used at the moment.

@317787106
Copy link
Contributor

@317787106 Use type to make the difference? ,see 83a6d58. What is type used for, I don't see it being used at the moment.

@halibobo1205 type is not used. May be it was used ago but not deleted later.

@halibobo1205
Copy link
Contributor

@317787106 Help figure out what this field was used for before, or add a new field?

@alexqrid
Copy link
Author

alexqrid commented Sep 7, 2023

Seems that type was kept to make API Ethereum JSON-RPC compatible.
And I agree with @halibobo1205 if we want to make use of various value types, the type should be defined and documented.
But what will be the number of type for each transaction, the same as protobuf number of enum ContractType?

@alexqrid
Copy link
Author

alexqrid commented Sep 7, 2023

Also I'm not quite sure with correctness of this part.
Imagine if one uses json rpc to parse transacted value and construct the following event:

{
"sender" : response.result.from,
"recipient": response.result.to,
"value": response.result.value
}

And this would be wrong for ShieldedTransaction, as the value can be either from or to, currently actual information for ShieldedTransaction types isn't displayed correctly for such a case.

@317787106
Copy link
Contributor

317787106 commented Sep 8, 2023

Seems that type was kept to make API Ethereum JSON-RPC compatible. And I agree with @halibobo1205 if we want to make use of various value types, the type should be defined and documented. But what will be the number of type for each transaction, the same as protobuf number of enum ContractType?

@alexqrid You are right, type is the transaction type. There are 3 types of transaction in ethereum, includes Legacy transactions、Access list transactions、EIP-1559 transactions. More details on https://docs.infura.io/networks/ethereum/concepts/transaction-types. 0x0 refers to Legacy transactions, and tron only has Legacy transactions. Column type can get from TransactionReceipt when format in jsonrpc and it cannot be deleted.

@317787106
Copy link
Contributor

@alexqrid Any transaction's type in java-tron is 0x0.

@317787106
Copy link
Contributor

317787106 commented Sep 8, 2023

@alexqrid The reason that we use amount in TransferAssetContract and other contract is to be compatible with tronscan and trongrid api. You can look at this https://apilist.tronscanapi.com/api/transaction-info?hash=af1094b946e947795656a9df3da7fef311b790c6ee067b29251b13fd5463c510. There is column amount. I am not sure if there is any problem if not used.

@halibobo1205
Copy link
Contributor

@alexqrid For compatibility reasons, no changes will be considered for now, cc @317787106 @jwrct .

@halibobo1205
Copy link
Contributor

Also I'm not quite sure with correctness of this part. Imagine if one uses json rpc to parse transacted value and construct the following event:

{
"sender" : response.result.from,
"recipient": response.result.to,
"value": response.result.value
}

And this would be wrong for ShieldedTransaction, as the value can be either from or to, currently actual information for ShieldedTransaction types isn't displayed correctly for such a case.

@317787106 @jwrct Please Take A Look

@halibobo1205
Copy link
Contributor

Seems that type was kept to make API Ethereum JSON-RPC compatible. And I agree with @halibobo1205 if we want to make use of various value types, the type should be defined and documented. But what will be the number of type for each transaction, the same as protobuf number of enum ContractType?

I think this is a good idea, let's discuss it, maybe we could add a new field. @317787106 @jwrct

@317787106
Copy link
Contributor

@alexqrid There may be compatible problem with previous version. Let's suppose that one user has used this value (same as amount) of the api eth_getTransactionByHash, if your changes take effect, vaule will be zero except TRX related. If this user don't acquire this changes, the data will be wrong.

If you only want value only related to TRX, you can use other api to get the ContractType. This is not time consuming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants