style: simplify code #7

dristpunk · 2024-02-20T11:45:44Z

No description provided.

solidity/contracts/ConnextVestingWallet.sol

solidity/test/integration/LlamaVesting.t.sol

solidity/interfaces/IConnextVestingWallet.sol

solidity/contracts/ConnextVestingWallet.sol

solidity/test/integration/LlamaVesting.t.sol

wei3erHase · 2024-02-20T17:02:18Z

solidity/contracts/ConnextVestingWallet.sol

- function vestedAmount(uint64) public view virtual override returns (uint256 _amount) {
- return 0;
+ constructor(address _owner, uint256 _totalAmount) Ownable(_owner) {
+ CLIFF = VESTING_START_DATE + VESTING_CLIFF_DURATION;


CLIFF is basically VESTING_START_DATE + 1 year no?

CLIFF is basically VESTING_START_DATE + 1 year no?

VESTING_START_DATE + 1 month. It's used just once so we could replace it with VESTING_START_DATE + VESTING_CLIFF_DURATION in the code

ahh ok, i think is good there, mmmm maybe rename to VESTING_CLIFF ?

VESTING_CLIFF

I'd remove it at all or moved its declaration to constants. Setting constants in constructor doesn't make sense

wei3erHase · 2024-02-20T17:05:14Z

solidity/interfaces/IConnextVestingWallet.sol

+
+ /**
+ * @notice NEXT tokens released
+ * @param _token The address of the NEXT token


*of the released ERC20 token

solidity/interfaces/IConnextVestingWallet.sol

wei3erHase · 2024-02-20T17:06:45Z

solidity/interfaces/IConnextVestingWallet.sol

+ /**
+ * @notice Sept 5th 2023 in seconds
+ * @return _timedelta The timestamp of Sept 5th 2023
+ */
+ function SEPT_05_2023() external view returns (uint64 _timedelta);
+


not a timedelta, unix timestamp!

wei3erHase · 2024-02-20T17:07:57Z

solidity/interfaces/IConnextVestingWallet.sol

+
+ /**
+ * @notice Vesting warmup time
+ * @dev 11 months offset


i'm not sure if @dev comments are correct in the interface, because they're talking about the immutable variable that's on the contract, it doesn't make sense to assume in the interface that this will return 11 months offset

i'd move the @dev comments to the contract

i'm not sure if @dev comments are correct in the interface, because they're talking about the immutable variable that's on the contract, it doesn't make sense to assume in the interface that this will return 11 months offset

i'd move the @dev comments to the contract

we could remove it because it doesn't make sense to add @dev natspec to constants. Or we can do // comments after them

wei3erHase · 2024-02-21T11:22:03Z

solidity/contracts/ConnextVestingWallet.sol

- */
- uint64 public constant VESTING_START_DATE = NEXT_TOKEN_LAUNCH + VESTING_OFFSET;
+ /// @inheritdoc IConnextVestingWallet
+ uint64 public constant VESTING_CLIFF = VESTING_START_DATE + VESTING_CLIFF_DURATION;


missing // Sept 5th 2024 comment

wei3erHase · 2024-02-21T11:22:27Z

solidity/interfaces/IConnextVestingWallet.sol

+ function VESTING_CLIFF_DURATION() external view returns (uint64 _timedelta);
+
+ /**
+ * @notice Vesting duration including one month of cliff


*including cliff duration

wei3erHase · 2024-02-21T12:34:22Z

solidity/contracts/ConnextVestingWallet.sol

- */
- address public constant NEXT_TOKEN = 0xFE67A4450907459c3e1FFf623aA927dD4e28c67a;
+ /// @inheritdoc IConnextVestingWallet
+ address public constant NEXT_TOKEN = 0xFE67A4450907459c3e1FFf623aA927dD4e28c67a; // Mainnet address


Mainnet NEXT token address

wei3erHase · 2024-02-21T12:35:35Z

solidity/contracts/ConnextVestingWallet.sol

+ function vestedAmount(uint64 _timestamp) public view returns (uint256 _amount) {
+ if (_timestamp < VESTING_CLIFF) {
+ return 0;
+ } else if (_timestamp >= VESTING_START) {


how come this is passing the tests?

Suggested change

} else if (_timestamp >= VESTING_START) {

} else if (_timestamp >= VESTING_END) {

wei3erHase · 2024-02-21T12:36:36Z

solidity/contracts/ConnextVestingWallet.sol


 /// @inheritdoc IConnextVestingWallet
- uint64 public immutable CLIFF;
+ uint64 public constant VESTING_START = VESTING_START_DATE + VESTING_DURATION;


WRONG NAMING!!

wei3erHase · 2024-02-21T13:50:54Z

solidity/contracts/ConnextVestingWallet.sol

- */
- uint64 public constant VESTING_OFFSET = ONE_YEAR - ONE_MONTH;
+ /// @inheritdoc IConnextVestingWallet
+ uint64 public constant VESTING_START = NEXT_TOKEN_LAUNCH + VESTING_OFFSET; // Aug 5th 2024


why the change pal?? it was ok Sept 5th - 1mth

wei3erHase · 2024-02-21T13:51:45Z

solidity/contracts/ConnextVestingWallet.sol

- STORAGE
- //////////////////////////////////////////////////////////////*/
+ /// @inheritdoc IConnextVestingWallet
+ uint64 public constant VESTING_END = VESTING_START + VESTING_DURATION; // Sept 5th 2025


i believe we should be talking about UNLOCK instead of VEST, @0xGorilla ?

wei3erHase · 2024-02-21T14:03:38Z

solidity/test/utils/Constants.sol

- uint64 public constant VESTING_DURATION = 365 days * 4;
+ uint64 public constant UNLOCK_DURATION = 365 days * 4;


NOOOOOOO!O!O!!O!O!O!O! hahhaha
this is actually the vest that goes into llamapay, it's 4ys, is a VEST

wei3erHase · 2024-02-21T14:04:02Z

solidity/test/integration/IntegrationBase.sol

- NEXT_TOKEN_ADDRESS, address(_connextVestingWallet), TOTAL_AMOUNT, VESTING_DURATION, AUG_01_2022, 0
+ NEXT_TOKEN_ADDRESS, address(_connextVestingWallet), TOTAL_AMOUNT, UNLOCK_DURATION, AUG_01_2022, 0


wrong naming ⚠️

wei3erHase

LGTM! 👍

turtlemoji

please change test pk from env to tests, and random would be better !

Also:

Encountered 1 failing test in solidity/test/integration/LlamaVesting.t.sol:IntegrationLlamaVesting
[FAIL. Reason: setup failed: environment variable "DEPLOYER_PRIVATE_KEY" not found] setUp() (gas: 0)

It seems it needs DEPLOYER_PRIVATE_KEY (the same you are setting on the action rn), but its not implemented on the .env.example so it fails locally. Adding it to the env fixes it tho.
Just to confirm recheck coverage for solidity/contracts/ConnextVestingWallet.sol. Are we ok with that %?
Also why MAX_DELTA is so high? (1 ether)
I would use just assertEq on all the cases. Only LlamaVesting tests error without delta.
Recheck all constants, like PAY_PER_SECOND. Some are not used anymore. Also the LlamaVesting.t.sol could use more comments explaining VestAndUnlock test numbers (Like in the other test)

Overall is pretty clean and secure, amazing job!

turtlemoji · 2024-02-23T10:16:47Z

solidity/test/integration/ConnextVestingWallet.t.sol

- assertApproxEqAbs(
- _connextVestingWallet.vestedAmount(NEXT_TOKEN_ADDRESS, _firstMilestoneTimestamp + MONTH), 2 ether, MAX_DELTA
- );
+ assertApproxEqAbs(_connextVestingWallet.vestedAmount(_firstMilestoneTimestamp + MONTH), 2 ether, MAX_DELTA);


Why MAX_DELTA is so high? (1 ether)
I would use just assertEq on this case and all this file cases

Why MAX_DELTA is so high? (1 ether) I would use just assertEq on this case and all this file cases

mostly because we use approximate datatimes, like 30 days = 1 month, but 1 month also = 1/12 year, which is 365 days.

But in this case I mean.
assertApproxEqAbs(_connextVestingWallet.vestedAmount(_firstMilestoneTimestamp + MONTH), 2 ether, MAX_DELTA);

You are comparing the vestedAmount, to 2 ether. With a delta of 1 ether. In this specific case it is wrong. If you use assertEq it will work just fine without delta

The feedback has been addressed 🐢

style: simplify code

c784cb4

dristpunk self-assigned this Feb 20, 2024

dristpunk added 2 commits February 20, 2024 14:51

style: lint and natspec

829319d

style: rm oz vesting wallet

90486ef

dristpunk requested review from 0xGorilla and wei3erHase February 20, 2024 12:58

dristpunk marked this pull request as ready for review February 20, 2024 12:58

0xGorilla reviewed Feb 20, 2024

View reviewed changes

solidity/contracts/ConnextVestingWallet.sol Outdated Show resolved Hide resolved

style: rm virtual

0d952b6

0xGorilla reviewed Feb 20, 2024

View reviewed changes

solidity/test/integration/LlamaVesting.t.sol Show resolved Hide resolved

0xGorilla reviewed Feb 20, 2024

View reviewed changes

solidity/interfaces/IConnextVestingWallet.sol Outdated Show resolved Hide resolved

wei3erHase reviewed Feb 20, 2024

View reviewed changes

solidity/interfaces/IConnextVestingWallet.sol Outdated Show resolved Hide resolved

wei3erHase reviewed Feb 20, 2024

View reviewed changes

solidity/contracts/ConnextVestingWallet.sol Outdated Show resolved Hide resolved

0xGorilla reviewed Feb 20, 2024

View reviewed changes

solidity/contracts/ConnextVestingWallet.sol Show resolved Hide resolved

style: move comments

8889c6c

wei3erHase reviewed Feb 20, 2024

View reviewed changes

solidity/test/integration/LlamaVesting.t.sol Outdated Show resolved Hide resolved

dristpunk added 2 commits February 20, 2024 17:21

style: rm unused var

83cb46e

style: replace natspec

e1b129f

dristpunk requested review from wei3erHase and 0xGorilla February 20, 2024 14:29

fix: add event

3428123

wei3erHase reviewed Feb 20, 2024

View reviewed changes

solidity/interfaces/IConnextVestingWallet.sol Show resolved Hide resolved

wei3erHase reviewed Feb 20, 2024

View reviewed changes

dristpunk added 2 commits February 20, 2024 20:29

style: fix natspec

c970ed6

style: resolve comments

e384723

wei3erHase reviewed Feb 21, 2024

View reviewed changes

fix: resolve comments

ab49279

wei3erHase reviewed Feb 21, 2024

View reviewed changes

style: fix naming

59fe8ca

wei3erHase reviewed Feb 21, 2024

View reviewed changes

fix: naming

b29a687

wei3erHase previously approved these changes Feb 21, 2024

View reviewed changes

turtlemoji previously requested changes Feb 22, 2024

View reviewed changes

turtlemoji reviewed Feb 23, 2024

View reviewed changes

fix: resolve comments

46c064d

dristpunk dismissed wei3erHase’s stale review via 46c064d February 23, 2024 14:07

0xGorilla previously approved these changes Feb 23, 2024

View reviewed changes

style: rm unused var, add natspec

a25e28f

dristpunk dismissed 0xGorilla’s stale review via a25e28f February 23, 2024 14:17

gas1cent approved these changes Feb 23, 2024

View reviewed changes

gas1cent merged commit 7a8e283 into dev Feb 23, 2024
3 checks passed

gas1cent deleted the style/simplify-contracts branch February 23, 2024 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: simplify code #7

style: simplify code #7

dristpunk commented Feb 20, 2024

wei3erHase Feb 20, 2024

dristpunk Feb 20, 2024

wei3erHase Feb 20, 2024 •

edited

Loading

dristpunk Feb 20, 2024 •

edited

Loading

wei3erHase Feb 20, 2024

wei3erHase Feb 20, 2024

wei3erHase Feb 20, 2024

dristpunk Feb 20, 2024

wei3erHase Feb 21, 2024

wei3erHase Feb 21, 2024

wei3erHase Feb 21, 2024

wei3erHase Feb 21, 2024

wei3erHase Feb 21, 2024

wei3erHase Feb 21, 2024

wei3erHase Feb 21, 2024

wei3erHase Feb 21, 2024

wei3erHase Feb 21, 2024 •

edited

Loading

wei3erHase left a comment

turtlemoji left a comment •

edited

Loading

turtlemoji Feb 23, 2024 •

edited

Loading

dristpunk Feb 23, 2024 •

edited

Loading

turtlemoji Feb 23, 2024 •

edited

Loading

	} else if (_timestamp >= VESTING_START) {
	} else if (_timestamp >= VESTING_END) {

		uint64 public constant VESTING_DURATION = 365 days * 4;
		uint64 public constant UNLOCK_DURATION = 365 days * 4;

		NEXT_TOKEN_ADDRESS, address(_connextVestingWallet), TOTAL_AMOUNT, VESTING_DURATION, AUG_01_2022, 0
		NEXT_TOKEN_ADDRESS, address(_connextVestingWallet), TOTAL_AMOUNT, UNLOCK_DURATION, AUG_01_2022, 0

style: simplify code #7

style: simplify code #7

Conversation

dristpunk commented Feb 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wei3erHase Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

dristpunk Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wei3erHase Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

wei3erHase left a comment

Choose a reason for hiding this comment

turtlemoji left a comment • edited Loading

Choose a reason for hiding this comment

turtlemoji Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

dristpunk Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

turtlemoji Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

wei3erHase Feb 20, 2024 •

edited

Loading

dristpunk Feb 20, 2024 •

edited

Loading

wei3erHase Feb 21, 2024 •

edited

Loading

turtlemoji left a comment •

edited

Loading

turtlemoji Feb 23, 2024 •

edited

Loading

dristpunk Feb 23, 2024 •

edited

Loading

turtlemoji Feb 23, 2024 •

edited

Loading