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

[Arbitrary Txs] Fix array encoding issue #4126

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jakubcolony
Copy link
Collaborator

@jakubcolony jakubcolony commented Jan 15, 2025

Description

This PR fixes an issue with array encoding when creating arbitrary transactions. The cause of the issue was passing a string representation of an array (e.g. "[]") instead of a "first-class" JS array.

It also improves the decoding of array arguments when showing a completed action.

Testing

To ensure thorough coverage, we'll deploy a test contract that has a function with nearly all possible argument types.

Deploying test contract

  1. Ensure your dev env is fully running.
  2. Create a new file called TestContract.sol in your CDapp root directory with the following contents:
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract TestContract {
    function testFunction(
        uint256 uint256Arg,
        uint256[] memory uint256ArrayArg,
        uint256[][] memory uint256NestedArrayArg,
        bool boolArg,
        bool[] memory boolArrayArg,
        address addressArg,
        address[] memory addressArrayArg,
        bytes memory bytesArg,
        bytes[] memory bytesArrayArg,
        string memory stringArg,
        string[] memory stringArrayArg
    ) public {}
}
  1. Run the following in your terminal to copy the contract over to the network container and compile it:
docker cp TestContract.sol network:/colonyCDappBackend/colonyNetwork/contracts && docker exec -it -w /colonyCDappBackend/colonyNetwork network npx hardhat compile
  1. Start the hardhat console with npm run hardhat, then run the following code line-by-line:
cf = await ethers.getContractFactory('TestContract')
c = await cf.deploy()
c.address

The last expression will print the address of the newly deployed contract. Copy it for later use.

Testing arbitrary transactions

  1. Create new arbitrary transaction action.
  2. In the modal, paste the deployed contract address and the following JSON ABI:
[
  {
    "inputs": [
      {
        "internalType": "uint256",
        "name": "uint256Arg",
        "type": "uint256"
      },
      {
        "internalType": "uint256[]",
        "name": "uint256ArrayArg",
        "type": "uint256[]"
      },
      {
        "internalType": "uint256[][]",
        "name": "uint256NestedArrayArg",
        "type": "uint256[][]"
      },
      {
        "internalType": "bool",
        "name": "boolArg",
        "type": "bool"
      },
      {
        "internalType": "bool[]",
        "name": "boolArrayArg",
        "type": "bool[]"
      },
      {
        "internalType": "address",
        "name": "addressArg",
        "type": "address"
      },
      {
        "internalType": "address[]",
        "name": "addressArrayArg",
        "type": "address[]"
      },
      {
        "internalType": "bytes",
        "name": "bytesArg",
        "type": "bytes"
      },
      {
        "internalType": "bytes[]",
        "name": "bytesArrayArg",
        "type": "bytes[]"
      },
      {
        "internalType": "string",
        "name": "stringArg",
        "type": "string"
      },
      {
        "internalType": "string[]",
        "name": "stringArrayArg",
        "type": "string[]"
      }
    ],
    "name": "testFunction",
    "outputs": [],
    "stateMutability": "nonpayable",
    "type": "function"
  }
]
  1. Select testFunction from the dropdown, and fill the fields with correct values according to their type. For example:
  • uint256Arg: 123456789
  • uint256ArrayArg: [123, 456, 789]
  • uint256NestedArrayArg: [[1, 2], [3, 4, 5]]
  • boolArg: true
  • boolArrayArg: [true, false, true]
  • addressArg: 0x742d35Cc6634C0532925a3b844Bc454e4438f44e
  • addressArrayArg: [0x742d35Cc6634C0532925a3b844Bc454e4438f44e, 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4]
  • bytesArg: 0xFF
  • bytesArrayArg: [0xFF, 0xAA]
  • stringArg: Hello World
  • stringArrayArg: ["Hello", "World", "Blockchain"]
  1. Submit the form. The transaction should go through and you should see the decoded arguments in the temporary completed action view.

  2. Repeat the steps using reputation or multi-sig decision method (both use the same code).

image

Resolves #4124

@jakubcolony jakubcolony self-assigned this Jan 15, 2025
* Parses a string representation of an array into an array of values.
* It supports nested arrays and handles strings within arrays.
*/
const getArrayFromString = (value: string, type: string) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is loosely based on Dapp implementation but has further improvements such as actually parsing nested arrays and handling potentially confusing strings containing commas and brackets (e.g. ["[", ","]).

@jakubcolony jakubcolony marked this pull request as ready for review January 15, 2025 23:00
@jakubcolony jakubcolony requested a review from a team as a code owner January 15, 2025 23:00
Nortsova
Nortsova previously approved these changes Jan 16, 2025
Copy link
Contributor

@Nortsova Nortsova left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for fixing this bug and testing every possible arg type 👏

Created a Custom transaction with all following arguments:
image

Successfully decoded:
image

Multi-sig works good too:
image

}

if (!value.startsWith('[') || !value.endsWith(']')) {
throw new Error('Invalid array format');
Copy link
Contributor

Choose a reason for hiding this comment

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

I would propose moving this to translation.

@jakubcolony jakubcolony linked an issue Jan 19, 2025 that may be closed by this pull request
Base automatically changed from feat/arbitrary-txs-network-bump to master January 20, 2025 10:48
@jakubcolony jakubcolony dismissed Nortsova’s stale review January 20, 2025 10:48

The base branch was changed.

@jakubcolony jakubcolony requested a review from Nortsova January 20, 2025 21:05
Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Amazing work @jakubcolony 🥇

Created a custom transaction via Permissions with the provided values
Screenshot 2025-01-21 at 10 30 59
Created another custom transaction via Reputation
Screenshot 2025-01-21 at 10 32 58

Really nice implementation for the getArrayFromString utils 👍

Copy link
Contributor

@Nortsova Nortsova left a comment

Choose a reason for hiding this comment

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

Reapproving after base branch changed ✅

image

Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

Great job with this and thanks for providing the test contract, super helpful! Nice implementation of getArrayFromString too.

Screenshot 2025-01-21 at 11 42 07

Screenshot 2025-01-21 at 11 42 33

Screenshot 2025-01-21 at 11 44 39

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.

[Arbitrary Txs] Array encoding issue
4 participants