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

improve visibility config #2066

Merged
merged 3 commits into from
Oct 1, 2024
Merged

improve visibility config #2066

merged 3 commits into from
Oct 1, 2024

Conversation

tudor-malene
Copy link
Collaborator

Why this change is needed

To improve the developer experience when configuring the visibility rules

What changes were made as part of this PR

Add syntactic sugar to the configuration

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required
PR checks

  • PR checks reviewed and performed

bool topic2CanView; // same
bool topic3CanView; // same
bool visibleToSender; // if true, the tx signer will see this event. Default false
uint256 eventSignature;
Copy link
Contributor

Choose a reason for hiding this comment

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

why uint256

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops. good spot

t2 := relevantToMap[topic2]
t3 := relevantToMap[topic3]
s := relevantToMap[sender]
cfg.EventConfigs[logConfig.EventSignature] = &core.EventVisibilityConfig{
Copy link
Contributor

Choose a reason for hiding this comment

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

Those really need a comment with examples to better understand event visibility.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the comments are in the config. This is the implementation.

Aren't the comments in ContractTransparencyConfig.solenough?

// erc20 transfer
configs[0] = EventLogConfig(hex"ddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef", false, true, true, false, false);
return VisibilityConfig(false, configs);
bytes32 eventSig = hex"ddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef";
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this hash from?

you can do "var visibilityRulesEventSignature = keccak256("preimage")"

Copy link
Contributor

@StefanIliev545 StefanIliev545 left a comment

Choose a reason for hiding this comment

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

LGTM, but the minor comments might be good to address beforehand if no other PR incoming here

@tudor-malene tudor-malene merged commit 9888300 into main Oct 1, 2024
3 checks passed
@tudor-malene tudor-malene deleted the tudor/improve_visib_cfg branch October 1, 2024 08:34
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