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

Feat/factory supports parameter within struct #1343

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Jds-23
Copy link

@Jds-23 Jds-23 commented Dec 16, 2024

[Not Complete Yet]
@typedarray

Copy link
Collaborator

@typedarray typedarray left a comment

Choose a reason for hiding this comment

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

Great start, left some in-progress comments

@@ -6,6 +6,21 @@ const llamaFactoryEventAbiItem = parseAbiItem(
"event LlamaInstanceCreated(address indexed deployer, string indexed name, address llamaCore, address llamaExecutor, address llamaPolicy, uint256 chainId)",
);

const FactoryEventSimpleParamsAbiItem = parseAbiItem([
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. I think you need to use parseAbi here because there are multiple items
  2. We use camelCase for most identifiers

Copy link
Author

Choose a reason for hiding this comment

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

  1. In case of function/event with struct, parseAbiItem can be used https://viem.sh/docs/abi/parseAbiItem#parameters . As parseAbiItem was already in use, I preferred to stick to it.
  2. Refactored, camelCase all the way. 🫡

"struct MarketParams {address loanToken; address collateralToken; address oracle; address irm; uint256 lltv;}",
]);

const FactoryEventWithDynamicChildParamsAbiItem = parseAbiItem([
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also include a test case where a struct/array is one of the indexed arguments and the user attempts to use one of its properties as the child address parameter. I think we'll have to validate against this - the address is not actually available in the event body because the value gets hashed and then stored as one of the topics.

Copy link
Author

Choose a reason for hiding this comment

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

Handled ✅

@Jds-23 Jds-23 force-pushed the feat/factory-supports-parameter-within-struct branch from d264faf to 211b0f6 Compare January 12, 2025 20:14
Jds-23 and others added 2 commits January 14, 2025 02:25
…the user attempts to use one of its properties as the child address parameter.
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.

2 participants