Skip to content

Conversation

@rkolpakov
Copy link
Contributor

No description provided.

@rkolpakov rkolpakov force-pushed the feature/top-up-allowed-tokens branch from a46dd3f to b575c80 Compare October 6, 2023 10:27
@rkolpakov rkolpakov force-pushed the feature/top-up-allowed-tokens branch from b575c80 to a3e9ac3 Compare October 6, 2023 10:35
@rkolpakov rkolpakov force-pushed the feature/top-up-allowed-tokens branch from 1937717 to f9d7b34 Compare October 9, 2023 07:46
@rkolpakov rkolpakov force-pushed the feature/top-up-allowed-tokens branch from f9d7b34 to a7eafa7 Compare October 9, 2023 07:47
@rkolpakov rkolpakov marked this pull request as ready for review October 9, 2023 08:02
@rkolpakov rkolpakov force-pushed the feature/top-up-allowed-tokens branch from e7e8c44 to dc03baa Compare October 24, 2023 10:15
@rkolpakov rkolpakov force-pushed the feature/top-up-allowed-tokens branch from dc03baa to 3b718ad Compare October 24, 2023 10:23
TheDZhon
TheDZhon previously approved these changes Oct 27, 2023
Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

LGTM 🍏

Left some nitpicks and rough thoughts only.

Given the following:

  • additional security measurements implemented with LIP-13 and preserved with this deployment as well
  • only the DAO Agent can change the token list

The risks are tolerable and good to go from my side.

// plus 1 because index 0 means a value is not in the set.
mapping(address => uint256) private allowedTokenIndices;

/// @notice Precise number of tokens in the system
Copy link
Contributor

Choose a reason for hiding this comment

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

the comment is misleading a bit (the definition is close to 'total supply')

return allowedTokens;
}

/// @notice Transforms amout from token format to precise format
Copy link
Contributor

Choose a reason for hiding this comment

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

typo 'amout'

@@ -0,0 +1,130 @@
// SPDX-FileCopyrightText: 2022 Lido <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

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

old copyright (not only in this file)

import "OpenZeppelin/[email protected]/contracts/access/AccessControl.sol";
import "OpenZeppelin/[email protected]/contracts/token/ERC20/extensions/IERC20Metadata.sol";

contract AllowedTokensRegistry is AccessControl {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not AccessControlEnumerable?

return _tokenAmount * 10 ** (DECIMALS - tokenDecimals);
}

/// @notice Returns precision of the token
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit misleading comment

address indexed topUpAllowedRecipients,
address trustedCaller,
address allowedRecipientsRegistry,
address allowedTokenssRegistry,
Copy link
Contributor

Choose a reason for hiding this comment

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

typo tokenss

Copy link
Contributor

Choose a reason for hiding this comment

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

there some memory[] external func args left still

);
}

function deployAllowedTokensRegistry(
Copy link
Contributor

Choose a reason for hiding this comment

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

not a strong point: maybe semantically, it could have been a dedicated factory for the tokens registry (since doesn't affect allowed recipients)

image


def external_contracts(network=DEFAULT_NETWORK):
if network == "mainnet" or network == "mainnet-fork":
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

"usdt"?

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.

7 participants