Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

contracts: Expose environment types for offchain tooling #14750

Merged
merged 6 commits into from
Aug 23, 2023

Conversation

athei
Copy link
Member

@athei athei commented Aug 10, 2023

In order to implement use-ink/cargo-contract#1167 we need to expose all types passed between pallet-contracts and the deployed contracts. The idea is to bundle all relevant types in an Environment struct mirroring the ink! Environment type. In order to expose it we add it to the config trait as Get<Environment>. The types are obviously only mentioned as PhantomData because we don't ever want to construct them. I did just this in this PR.

I think this would be an elegant solution if it wouldn't be for paritytech/scale-info#111. @ascjones Can we maybe add an attribute to scale-info that allows to opt-in to PhantomData types?

cc @SkymanOne

cumulus companion: paritytech/cumulus#3036

@athei athei requested a review from a team August 10, 2023 21:43
@athei athei changed the title contracts: Expose environment types for offchain tooling WIP: contracts: Expose environment types for offchain tooling Aug 10, 2023
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

You could use a wrapper type e.g.:

#[derive(TypeInfo)]
struct EnvironmentType<T>(PhantomData<T>)

pub struct Environment<T: Config> {
	account_id: EnvironmentType<AccountIdOf<T>>,

@athei
Copy link
Member Author

athei commented Aug 15, 2023

Hmm yes that could work. This whole solution seems a bit hacky. Even without the wrapper type. But I guess our only way to get this in without introducing a whole new way of just expressing types into the metadata.

@athei athei added A0-please_review Pull request needs code review. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. C1-low PR touches the given topic and has a low impact on builders. labels Aug 15, 2023
@athei athei changed the title WIP: contracts: Expose environment types for offchain tooling contracts: Expose environment types for offchain tooling Aug 15, 2023
@athei
Copy link
Member Author

athei commented Aug 15, 2023

Implemented your idea. Seems to be working. This is the output. I think we can work with that:

{
  "id": 576,
  "type": {
    "path": [
      "pallet_contracts",
      "Environment"
    ],
    "params": [
      {
        "name": "T",
        "type": null
      }
    ],
    "def": {
      "composite": {
        "fields": [
          {
            "name": "account_id",
            "type": 577,
            "typeName": "EnvironmentType<AccountIdOf<T>>"
          },
          {
            "name": "balance",
            "type": 578,
            "typeName": "EnvironmentType<BalanceOf<T>>"
          },
          {
            "name": "hash",
            "type": 579,
            "typeName": "EnvironmentType<<T as frame_system::Config>::Hash>"
          },
          {
            "name": "hasher",
            "type": 580,
            "typeName": "EnvironmentType<<T as frame_system::Config>::Hashing>"
          },
          {
            "name": "timestamp",
            "type": 582,
            "typeName": "EnvironmentType<MomentOf<T>>"
          },
          {
            "name": "block_number",
            "type": 583,
            "typeName": "EnvironmentType<BlockNumberFor<T>>"
          }
        ]
      }
    }
  }
},
{
  "id": 577,
  "type": {
    "path": [
      "pallet_contracts",
      "EnvironmentType"
    ],
    "params": [
      {
        "name": "T",
        "type": 0
      }
    ],
    "def": {
      "composite": {}
    }
  }
},
{
  "id": 578,
  "type": {
    "path": [
      "pallet_contracts",
      "EnvironmentType"
    ],
    "params": [
      {
        "name": "T",
        "type": 6
      }
    ],
    "def": {
      "composite": {}
    }
  }
},
{
  "id": 579,
  "type": {
    "path": [
      "pallet_contracts",
      "EnvironmentType"
    ],
    "params": [
      {
        "name": "T",
        "type": 12
      }
    ],
    "def": {
      "composite": {}
    }
  }
},
{
  "id": 580,
  "type": {
    "path": [
      "pallet_contracts",
      "EnvironmentType"
    ],
    "params": [
      {
        "name": "T",
        "type": 581
      }
    ],
    "def": {
      "composite": {}
    }
  }
},

@ascjones
Copy link
Contributor

Hmm yes that could work. This whole solution seems a bit hacky. Even without the wrapper type. But I guess our only way to get this in without introducing a whole new way of just expressing types into the metadata.

A little bit hacky, perhaps. But a very neat way of getting the information we need into the metadata without adding any additional configuration schema. Of course the downstream tooling will need to do some explicit name lookups to pull out the types, but I think it might be worth it to keep things simple on the node side.

Copy link

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

Yes, we can definitely work with this

@athei
Copy link
Member Author

athei commented Aug 20, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@athei
Copy link
Member Author

athei commented Aug 20, 2023

Please also approve the companion.

@athei
Copy link
Member Author

athei commented Aug 23, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@athei
Copy link
Member Author

athei commented Aug 23, 2023

bot merge

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants