Skip to content

Comments

Touchups#175

Draft
vzotova wants to merge 8 commits intomainfrom
touchups
Draft

Touchups#175
vzotova wants to merge 8 commits intomainfrom
touchups

Conversation

@vzotova
Copy link
Contributor

@vzotova vzotova commented Feb 10, 2026

No description provided.

Copy link
Member

@cygnusv cygnusv left a comment

Choose a reason for hiding this comment

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

Looks good in general! just some comments

Comment on lines +499 to +503
ApplicationInfo storage applicationStruct = applicationInfo[msg.sender];
require(
applicationStruct.status == ApplicationStatus.APPROVED,
"Application is not approved"
);
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this check? Given that TACoApp is trusted in this context, it seems a bit redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to get same logic as in another methods, and that part is already tested

Copy link
Member

Choose a reason for hiding this comment

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

I see. My concern was cost for a migration of dozens of stakers, but apparently since we do this in a batch TX, only the first access to applicationStruct.status would get the full SLOAD cost, and the rest will only be 100 gas.

Comment on lines +306 to +312
// Unstake
stakingProviderStruct.tStake -= authorization.deauthorizing;
decreaseStakeCheckpoint(stakingProvider, authorization.deauthorizing);
emit Unstaked(stakingProvider, authorization.deauthorizing);
token.safeTransfer(stakingProviderStruct.owner, authorization.deauthorizing);

authorization.deauthorizing = 0;
Copy link
Member

Choose a reason for hiding this comment

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

What's the goal of these lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unstake the rest, everything over minimum

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