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

[Katana] Allow choosing the class in the genesis file without affecting the actual class hash #1502

Closed
kariy opened this issue Feb 1, 2024 · 11 comments
Assignees
Labels
contributor enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed katana This issue is related to Katana

Comments

@kariy
Copy link
Member

kariy commented Feb 1, 2024

An improvement to how one can specify a class to be used by a contract in the genesis file as suggested in #1483.

Currently, when writing the genesis file, you can specify the allocated accounts/fee token/udc to use a user-defined class instead of the default class provided by Katana. This is done by specifying exactly the class hash of the class you want to use in the class field of the contract entry.

Example of a genesis file, where the fee token contract is set to use a custom contract class.

{
  ..
 
  "feeToken": {
 "address": "0x55",
 "name": "ETHER",
 "symbol": "ETH",
 "decimals": 18,
        // This field is optional, if not provided the default class will be used.
        // If specified, the value must be one the class hash defined in the `classes` field below. Otherwise, an error will be returned.
 "class": "0xdeadbeef"
  },
  "classes": [
    {
      "path": "../path/to/class/file",
      // This field is optional, is used to override the class hash value of the class itself.
      // By default, the class hash will be computed from the class at `path`.
      "classHash": "0xdeadbeef"
    }
  ]
 
  ..
}

This method requires the user to input the exact class hash. For instance, if a class at "../path/to/class/file" computes to a hash of 0x1234, but 0xdeadbeef is specified in the genesis file, the latter becomes the canonical class hash. So, ideally we should favour an approach that doesn't involve affecting the resultant class hash.

To improve flexibility and avoid affecting the resultant class hash, a suggestion from the aforementioned PR

Trying to play around with classes, when I don't define the class hash of a class entry inside the genesis classes, it means I have to provider the exact class hash into the allocations. With the current implem, it may be possible to add a name field to the GenesisClassJson and allow a lookup by name. Does it make sense to you too?

recommends introducing a name field in [GenesisClassJson](https://github.com/dojoengine/dojo/blob/042a2c0d3659d76e08091919971573805c503f70/crates/katana/primitives/src/genesis/json.rs#L40). This field would allow class identification by a unique name rather than by hash, specifically for lookup purposes within the classes section of the genesis file. The scope of the field should only be limited to the lookup functionality in the classes section of the JSON file and should not have any significant outside of that.

{
  "feeToken": {
	"address": "0x55",
	"name": "ETHER",
	"symbol": "ETH",
	"decimals": 18,
        // This field is optional, if not provided the default class will be used.
        // If specified, the value must be one the class hash defined in the `classes` field below. Otherwise, an error will be returned.
	"class": "MyFeeTokenContract"
  },
  "classes": [
    {
      "path": "../path/to/class/file",
      // This field is optional, is used to override the class hash value of the class itself.
      // By default, the class hash will be computed from the class at `path`.
      "name": "MyFeeTokenContract"
    }
  ]
}
@kariy kariy added enhancement New feature or request help wanted Extra attention is needed katana This issue is related to Katana labels Feb 1, 2024
@kariy kariy added the good first issue Good for newcomers label Feb 1, 2024
@Yogalholic
Copy link
Contributor

I would like to give it a try please.

@kariy
Copy link
Member Author

kariy commented Feb 1, 2024

I would like to give it a try please.

Assigned!

@kariy
Copy link
Member Author

kariy commented Feb 1, 2024

@Yogalholic don't hesitate to ask question/help about the implementation

@Yogalholic
Copy link
Contributor

@Yogalholic don't hesitate to ask question/help about the implementation

Ok, I'm used to contribute on Madara where we use cargo to compile/test but on dojo we have to compile with scarb instead?

@glihm
Copy link
Collaborator

glihm commented Feb 3, 2024

@Yogalholic don't hesitate to ask question/help about the implementation

Ok, I'm used to contribute on Madara where we use cargo to compile/test but on dojo we have to compile with scarb instead?

Nope, it's like you're used to do with madara, to compile the project and run the tests dojo also uses cargo.

# All the project
cargo build

# Running katana for instance
cargo run --bin katana

# Run the tests
cargo nextest run --all-features

Scarb is used internally by Sozo, so usually you don't use scarb explicitly when working with dojo.

@kariy
Copy link
Member Author

kariy commented Feb 18, 2024

Hey, @Yogalholic. Any updates on this? Let me know if there's something blocking you.

@Yogalholic
Copy link
Contributor

It took me some time to get accustomed to the Dojo/Katana stack. And my server needs one hour to compile dojo, that's why Im so long.
I've almost finished updating the json.rs file then I will have to update the tests. Do you wish me to open a draft PR to take a look at my work?

@kariy
Copy link
Member Author

kariy commented Feb 18, 2024

It took me some time to get accustomed to the Dojo/Katana stack. And my server needs one hour to compile dojo, that's why Im so long.

I see. Are you compiling the entire project? The scope of the issue doesn't expand outside of the katana-primitives crate, so you only ever need to compile that particular crate to make the compile time faster.

I've almost finished updating the json.rs file then I will have to update the tests. Do you wish me to open a draft PR to take a look at my work?

Yes, that would certainly be very helpful.

@Yogalholic
Copy link
Contributor

I've created the draft PR here

@Yogalholic
Copy link
Contributor

I've created the final PR here

kariy added a commit that referenced this issue May 22, 2024
… w/o affecting the actual class hash (#1975)

* resolving issue #1502

* run clippy

* create custom deserializer

* Fills up name_classes HashMap after hash classes computation

* stuff

---------

Co-authored-by: Ammar Arif <[email protected]>
@kariy
Copy link
Member Author

kariy commented May 22, 2024

resolved in #1975

@kariy kariy closed this as completed May 22, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Dojo May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed katana This issue is related to Katana
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants