Skip to content

Conversation

@zach030
Copy link

@zach030 zach030 commented Oct 7, 2025

No description provided.

@github-project-automation github-project-automation bot moved this to Todo in Tycho Oct 7, 2025
@zach030 zach030 changed the title feat: support disable token overwrite feat: support disabling token overwrite Oct 7, 2025
@zach030 zach030 marked this pull request as ready for review October 9, 2025 13:29
@zach030 zach030 force-pushed the feat/disable-token-overwrite branch from 75b46d0 to 709ddd9 Compare November 19, 2025 13:13
Copy link
Contributor

@zizou0x zizou0x left a comment

Choose a reason for hiding this comment

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

Thanks for this one! Just a few small comments

Comment on lines +524 to +527
for token in &self.disable_overwrite_tokens {
let overwrites = TokenProxyOverwriteFactory::new(*token, None);
balance_overwrites.extend(overwrites.get_overwrites())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand how this works, why do we need to create an empty overwrite factory? Can't we just remove entries from balance_overwrites?

Comment on lines +419 to +424
if self
.disable_overwrite_tokens
.contains(&addr)
{
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just remove them from the overwrites but keep balances stored?

This is because people will expect this to contain all the balances, it's a bit shady that we have this edge case that makes them being ignored.

let potential_rebase_tokens: HashSet<Address> = if let Some(bytes) = snapshot
.component
.static_attributes
.get("rebase_tokens")
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this name weird, I'll think about it but I think we should make it more obvious what it is

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

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

3 participants