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

Useless reference to MODULE_ID_INSTALLER #130

Open
alcueca opened this issue Feb 5, 2024 · 2 comments
Open

Useless reference to MODULE_ID_INSTALLER #130

alcueca opened this issue Feb 5, 2024 · 2 comments
Assignees

Comments

@alcueca
Copy link

alcueca commented Feb 5, 2024

The inclusion of MODULE_ID_INSTALLER in the call to getEndpointCreationCode in ReflexerDispatcher:L47 is confusing. Why do we pass an uint32 to the function? MODULE_ID_INSTALLER will always be 1, what would the overriding code do with that?.

@zerosnacks
Copy link
Contributor

The inclusion of MODULE_ID_INSTALLER in the call to getEndpointCreationCode in ReflexerDispatcher:L47 is confusing. Why do we pass an uint32 to the function? MODULE_ID_INSTALLER will always be 1, what would the overriding code do with that?.

It is best explained in the context of an overriding endpoint in Light

https://github.com/chromaxyz/light-core/blob/ffcf899e759b92ed6edc172a258e0d65e17aac6c/src/modules/Config.sol#L535-L547

It was implemented this way so that modules implementing the _getEndpointCreationCode can conditionally decide which creationCode to return. It is similar to common hook methods providing additional context so that the implementation can perform conditional logic.

In the current implementation of Light we only have 1 custom endpoint so this additional context is quite redundant.

For all single-modules (with single endpoints) we use the creationCode as specified by the Installer. For multi-modules it is expected for the endpoint to be created in a "factory" (in this case then adding of a new asset).

Consider the following case:

MULTI_MODULE_A (id: 100) and MULTI_MODULE_B (id: 101) being able to be spawned from a "factory" module. Both call _createEndpoint() in their configuration flow. To be able to deploy a different endpoint type for MULTI_MODULE_A than MULTI_MODULE_B we need to know their module id and then conditionally return the custom endpoint code.

Concluding: in Reflex, on its own, there is no need for this additional context but considering the overhead is very limited and the flexibility it gives is great therefore I think it is worth keeping.

@alcueca
Copy link
Author

alcueca commented Feb 6, 2024

I understand the inclusion of a parameter in getEndpointCreationCode, and maybe an uint32 moduleId is enough (isntead of a more generic bytes or bytes32 data).

Still, passing the MODULE_ID_INSTALLER in ReflexerDispatcher:L47 is confusing, and might be clearer to pass 0 as the parameter since it will be ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants