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

Adds autoIncrease flag to increase authorizations in topUp #155

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

vzotova
Copy link
Contributor

@vzotova vzotova commented Sep 29, 2023

Based on #153
With flag set to true each topUp will increase authorization amount for all authorized app

@github-actions
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/threshold-network/solidity-contracts/actions/runs/6357329563 check.

cygnusv
cygnusv previously approved these changes Nov 14, 2023
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.

LGTM! Just one comment: Why using the "toggle" approach instead of a setter? It's slightly less ergonomic, in my opinion, but interested to see what was your line of thinking.

contracts/staking/IStaking.sol Outdated Show resolved Hide resolved
function topUpNu(address stakingProvider) external;
/// @notice Toggle auto authorization increase flag. If true then all amount
/// in top-up will be added to already authorized applications.
function toggleAutoAuthorizationIncrease(address stakingProvider) external;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function toggleAutoAuthorizationIncrease(address stakingProvider) external;
function toggleAutoIncrease(address stakingProvider) external;

Copy link
Contributor Author

@vzotova vzotova Nov 14, 2023

Choose a reason for hiding this comment

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

I'm not sure about that, I want names explicit as possible. If you sure it's better then I'll update

contracts/staking/TokenStaking.sol Outdated Show resolved Hide resolved
Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/threshold-network/solidity-contracts/actions/runs/6864859193 check.

@vzotova
Copy link
Contributor Author

vzotova commented Nov 14, 2023

LGTM! Just one comment: Why using the "toggle" approach instead of a setter? It's slightly less ergonomic, in my opinion, but interested to see what was your line of thinking.

not strong opinion: from code perspective - if you call this tx you want to change parameter/toggle, there is no other options that you can change except to opposite value. In the same case setter should fail when parameter is equal.

Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/threshold-network/solidity-contracts/actions/runs/6864949782 check.

cygnusv
cygnusv previously approved these changes Nov 18, 2023
lukasz-zimnoch
lukasz-zimnoch previously approved these changes Nov 20, 2023
@cygnusv cygnusv dismissed stale reviews from lukasz-zimnoch and themself November 20, 2023 10:16

The merge-base changed after approval.

Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/threshold-network/solidity-contracts/actions/runs/6928617748 check.

Copy link

Solidity API documentation preview available in the artifacts of the https://github.com/threshold-network/solidity-contracts/actions/runs/6931780997 check.

@cygnusv cygnusv merged commit 116e669 into main Nov 20, 2023
11 checks passed
@cygnusv cygnusv deleted the auto-increase branch November 20, 2023 15:00
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.

4 participants