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

Feat: Add calldata flag to seer evm generator #115

Merged
merged 3 commits into from
Nov 25, 2024
Merged

Feat: Add calldata flag to seer evm generator #115

merged 3 commits into from
Nov 25, 2024

Conversation

karacurt
Copy link
Contributor

No description provided.

@karacurt karacurt requested a review from zomglings November 22, 2024 15:54
Copy link
Contributor

@zomglings zomglings left a comment

Choose a reason for hiding this comment

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

@karacurt : I have an issue with the current implementation.

Suppose I set --calldata on a transaction-sending command. Then it shouldn't require me to specify:

  1. --rpc
  2. --contract
  3. --keystore

It should just produce the same output as abi encoding the method selector with the arguments.

You can see how the current implementation doesn't satisfy this property using ownable-erc-721:

% ./ownable-erc-721 deploy --calldata
Error: --keystore not specified (this should be a path to an Ethereum account keystore file)
Usage:
  ownable-erc-721 deploy [flags]

Flags:
      --calldata                          Set this flag if want to return the calldata instead of sending the transaction
      --gas-limit uint                    Gas limit for the transaction
      --gas-price string                  Gas price to use for the transaction
  -h, --help                              help for deploy
      --keyfile string                    Path to the keystore file to use for the transaction
      --max-fee-per-gas string            Maximum fee per gas to use for the (EIP-1559) transaction
      --max-priority-fee-per-gas string   Maximum priority fee per gas to use for the (EIP-1559) transaction
      --name-0 string                     name-0 argument
      --nonce string                      Nonce to use for the transaction
      --owner string                      owner argument (common.Address)
      --password string                   Password to use to unlock the keystore (if not specified, you will be prompted for the password when the command executes)
      --rpc string                        URL of the JSONRPC API to use
      --safe string                       Address of the Safe contract
      --safe-api string                   Safe API for the Safe Transaction Service (optional)
      --safe-create-call string           Address of the CreateCall contract (optional)
      --safe-nonce string                 Safe nonce overrider for the transaction (optional)
      --safe-operation uint8              Safe operation type: 0 (Call) or 1 (DelegateCall) - default is 1 (default 1)
      --safe-predict-address              Predict the deployment address (only works for Safe transactions)
      --safe-salt string                  Salt to use for the Safe transaction
      --simulate                          Simulate the transaction without sending it
      --symbol string                     symbol argument
      --timeout uint                      Timeout (in seconds) for interactions with the JSONRPC API (default 60)
      --value string                      Value to send with the transaction

--keystore not specified (this should be a path to an Ethereum account keystore file)

Or

% ./ownable-erc-721 approve --calldata --to-0 0xe49a94044c101bC1fD1158B1Ae8D6893E99465D1 --token-id 5  
Error: --contract not specified
Usage:
  ownable-erc-721 approve [flags]

Flags:
      --calldata                          Set this flag if want to return the calldata instead of sending the transaction
      --contract string                   Address of the contract to interact with
      --gas-limit uint                    Gas limit for the transaction
      --gas-price string                  Gas price to use for the transaction
  -h, --help                              help for approve
      --keyfile string                    Path to the keystore file to use for the transaction
      --max-fee-per-gas string            Maximum fee per gas to use for the (EIP-1559) transaction
      --max-priority-fee-per-gas string   Maximum priority fee per gas to use for the (EIP-1559) transaction
      --nonce string                      Nonce to use for the transaction
      --password string                   Password to use to unlock the keystore (if not specified, you will be prompted for the password when the command executes)
      --rpc string                        URL of the JSONRPC API to use
      --safe string                       Address of the Safe contract
      --safe-api string                   Safe API for the Safe Transaction Service (optional)
      --safe-function string              Safe function overrider to use for the transaction (optional)
      --safe-nonce string                 Safe nonce overrider for the transaction (optional)
      --safe-operation uint8              Safe operation type: 0 (Call) or 1 (DelegateCall)
      --simulate                          Simulate the transaction without sending it
      --timeout uint                      Timeout (in seconds) for interactions with the JSONRPC API (default 60)
      --to-0 string                       to-0 argument (common.Address)
      --token-id string                   token-id argument
      --value string                      Value to send with the transaction

--contract not specified

@karacurt karacurt requested a review from zomglings November 22, 2024 18:18
Copy link
Contributor

@zomglings zomglings left a comment

Choose a reason for hiding this comment

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

Looks good in general, just two small changes remaining: cmd.Printf -> cmd.Println.


if calldata {
deployCalldataHex := hex.EncodeToString(deployCalldata)
cmd.Printf(deployCalldataHex)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use cmd.Println instead of cmd.Printf - you want to have the newline at the end of the print output as it makes the output more usable in a terminal window.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!


if calldata {
txCalldataHex := hex.EncodeToString(txCalldata)
cmd.Printf(txCalldataHex)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here, use cmd.Println instead of cmd.Printf. Just to clarify, this puts a newline at the end of the output and makes it easier to work with in a terminal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@karacurt karacurt requested a review from zomglings November 25, 2024 13:31
Copy link
Contributor

@zomglings zomglings left a comment

Choose a reason for hiding this comment

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

lgtm

great work @karacurt

@karacurt karacurt merged commit ced5f98 into main Nov 25, 2024
1 check passed
@karacurt karacurt deleted the feat/calldata branch November 25, 2024 14:23
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.

2 participants