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

Allow contractimpls across mods #1322

Merged
merged 9 commits into from
Sep 10, 2024
Merged

Allow contractimpls across mods #1322

merged 9 commits into from
Sep 10, 2024

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Aug 19, 2024

What

Add a new trait ContractFunctionRegister that wraps the existing register function that all contracts use to create a table of all their functions.

Why

When a contract is registered with the test environment it uses the table of functions to know what functions a contract has defined.

Today contract functions get added to the table by using the ctor crate to call the #mod::register function where #mod is a generated module at the same location as where the #[contract] macro gets used.

The problem with this, and that was reported in #1321, is that it's valid in Rust to place type implementations anywhere in a crate that is able to reference the type, but there's no way for the generated code to know what relative or absolute path to use to reference that #mod.

The generated code assumes that the #mod is imported into the current scope, which it never will be because users have no reason to manually do it because it is hidden from them and they don't know it exists.

This situation leads to the following error that makes little sense to a developer because it refers to hidden generated code:

use of undeclared crate or module `__Contract_fn_set_registry`

Most of the time when folks add additional impl blocks to a type they will import the type into the current module so as to reference it, and so we can improve the existing situation by shifting the call to #mod::register to be a call to Contract::register.

To ensure that the register function does not clash with a contract function that may also be named register the function is defined on a hidden trait, ContractFunctionRegister, and is only used in the context of that trait.

There are some other minor changes included that remove assumptions about the type name of a contract being a simple identifier, so that when it is more than an identifier, such as a path (e.g. path::Identifier), the full type name is preserved everywhere it is needed.

Close #1321

@leighmcculloch leighmcculloch changed the title Fix errors when contractimpls across mods Allow contractimpls across mods Aug 23, 2024
@leighmcculloch
Copy link
Member Author

TODO:

  • Make it possible to use the path keyword crate to reference the contract. For some reason using it is problematic because I think we combine it with super:: somewhere.
  • Find a way to auto-reference the contract client, because what happens is even if the user references the contract directly they still need to then import the contract client because of generated impls on the client.

@leighmcculloch leighmcculloch marked this pull request as ready for review September 6, 2024 03:07
@leighmcculloch
Copy link
Member Author

leighmcculloch commented Sep 9, 2024

I think we should merge this as is, even if there are some edge cases not covered, because it covers an 80% of what's required to get it to work, and the edge cases may require significantly more refactoring. So it'd be good to get the minimal merged so that the refactor can be broken up, and be easier to review.

We can create follow up issues after this change merges.

@leighmcculloch leighmcculloch requested review from a team and removed request for graydon September 10, 2024 06:47
Copy link
Contributor

@dmkozh dmkozh left a comment

Choose a reason for hiding this comment

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

That's really nice, thanks!

@leighmcculloch leighmcculloch added this pull request to the merge queue Sep 10, 2024
Merged via the queue into main with commit 0a86ece Sep 10, 2024
16 checks passed
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.

Splitting contract impls across modules leads to errors
2 participants