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

Consider renaming fields and changing types in the broadcast JSON output files #2987

Open
mds1 opened this issue Aug 28, 2022 · 3 comments
Open
Labels
C-forge Command: forge Cmd-forge-script Command: forge script T-feature Type: feature

Comments

@mds1
Copy link
Collaborator

mds1 commented Aug 28, 2022

Component

Forge

Describe the feature you would like

Right now in the broadcast logs there are two fields with names we should consider changing due to conflicts with language keywords.

  1. function is a keyword in JS, solidity, and likely other languages. I'd suggest renaming this to functionSig since the full signature is shown
  2. type is also a keyword in many languages. I'd suggest renaming to txType

There are also fields where the output types should be changed (I recall discussing this in the past but not sure if we're tracking anywhere so figured I'd mention it here). First, in the transactions array:

  • transaction.type should be a number, currently it's a hex string
  • transaction.gas should be a number, currently it's a hex string
  • Addresses are not logged as checksummed but they should be (also tracked in Checksum address output in log_address event and broadcast files #2975)
  • When a function argument is an address, the arguments array omits the 0x prefix but it should be included.
  • When a function argument is a boolean, the arguments array contains a string of "true" instead of a boolean of true (same for false)

In the receipts array:

  • transactionIndex can be a number, currently it's a hex string
  • blockNumber can be a number, currently it's a hex string
  • In the logs array, transactionIndex, logIndex, and transactionLogIndex can all be numbers, currently they are hex strings
  • cumulativeGasUsed can be a number, currently it's a hex string
  • gasUsed can be a number, currently it's a hex string
  • Addresses are not logged as checksummed but they should be
  • status is a hex string but should be either a boolean or a number of 1 or 0

The above list is not comprehensive and there may be other fields that should have a more readable type.

These suggestions deviate from the ethereum JSON RPC spec in the sense that all numbers are "quantities" which means they're returned from the node as hex strings with as few leading zeros as possible. However, this makes the data less readable and harder to parse, and I don't think there's a reason why these log files need to strictly follow the spec.

Additional context

No response

@Evalir
Copy link
Member

Evalir commented Jun 16, 2023

I believe this has been done as @DaniPopes pointed out, and quickly checking recent broadcast logs it seems done. Feel free to reopen if it's not!

@Evalir Evalir closed this as completed Jun 16, 2023
@mds1
Copy link
Collaborator Author

mds1 commented Jun 16, 2023

Hmm when was this done? I don't see a PR linked, and I just foundryup'd and ran a script, and I see none of these changes implemented except for the address arg now including the leading 0x 😅

{
  "transactions": [
    {
      "hash": "0xba6f75b3d3197fb86d9c22d8e21cf223dfc65d77dd1450046f6658a8dad10f1a",
      "transactionType": "CREATE",
      "contractName": "Counter",
      "contractAddress": "0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512",
      "function": null,
      "arguments": [
        "0x000000000000000000000000000000000000007B",
        "true"
      ],
      "transaction": {
        "type": "0x02",
        "from": "0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266",
        "gas": "0x23494",
        "value": "0x0",
        "data": "0x608060405234801561001057600080fd5b5060405161018a38038061018a83398101604081905261002f9161003a565b505060008055610085565b6000806040838503121561004d57600080fd5b82516001600160a01b038116811461006457600080fd5b6020840151909250801515811461007a57600080fd5b809150509250929050565b60f7806100936000396000f3fe6080604052348015600f57600080fd5b5060043610603c5760003560e01c80633fb5c1cb1460415780638381f58a146053578063d09de08a14606d575b600080fd5b6051604c3660046083565b600055565b005b605b60005481565b60405190815260200160405180910390f35b6051600080549080607c83609b565b9190505550565b600060208284031215609457600080fd5b5035919050565b60006001820160ba57634e487b7160e01b600052601160045260246000fd5b506001019056fea264697066735822122072cde5e6bbd063dda2aca7fbf2547afa87257af5368206714e2af92cb926239b64736f6c63430008140033000000000000000000000000000000000000000000000000000000000000007b0000000000000000000000000000000000000000000000000000000000000001",
        "nonce": "0x1",
        "accessList": []
      },
      "additionalContracts": [],
      "isFixedGasLimit": false
    }
  ],
  "receipts": [
    {
      "transactionHash": "0xba6f75b3d3197fb86d9c22d8e21cf223dfc65d77dd1450046f6658a8dad10f1a",
      "transactionIndex": "0x0",
      "blockHash": "0xb8238f8936f15e7dc0d86898e487cce2c828d0e343dc68870cfc6c1d693c59da",
      "blockNumber": "0x2",
      "from": "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266",
      "to": null,
      "cumulativeGasUsed": "0x1b269",
      "gasUsed": "0x1b269",
      "contractAddress": "0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512",
      "logs": [],
      "status": "0x1",
      "logsBloom": "0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
      "type": "0x2",
      "effectiveGasPrice": "0xe7056143"
    }
  ],
  "libraries": [],
  "pending": [],
  "returns": {},
  "timestamp": 1686954856,
  "chain": 31337,
  "multi": false,
  "commit": "20a75e1"
}

@mds1 mds1 reopened this Jun 16, 2023
@Evalir
Copy link
Member

Evalir commented Jun 16, 2023

ahhh my bad, i might have closed this issue by mistake instead of a similar one 😅 this one's still on the todo list

@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@zerosnacks zerosnacks removed this from the v1.0.0 milestone Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge Cmd-forge-script Command: forge script T-feature Type: feature
Projects
Status: Todo
Development

No branches or pull requests

4 participants