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

feat: ynViewer upgrade #119

Merged
merged 9 commits into from
Jul 3, 2024
Merged

feat: ynViewer upgrade #119

merged 9 commits into from
Jul 3, 2024

Conversation

johnnyonline
Copy link
Member

No description provided.

@johnnyonline johnnyonline requested a review from danoctavian June 27, 2024 21:39
src/ynViewer.sol Outdated
IynETH public ynETH;
IStakingNodesManager public stakingNodesManager;

IERC4626 public immutable ynETH;
Copy link
Contributor

Choose a reason for hiding this comment

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

ynETH is not a IERC4626 for now and may never be one, it doesn't break any of the logic here, but i'd rather not confuse.

Let's leave it with IynETH

Copy link
Member Author

Choose a reason for hiding this comment

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

src/ynViewer.sol Outdated
function getRate() external view returns (uint256) {
uint256 _totalSupply = ynETH.totalSupply();
uint256 _totalAssets = ynETH.totalAssets();
if (_totalSupply == 0 || _totalAssets == 0) return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

If any of these are 0 at the initial system state, the rate is actually 1.

Since ynETH:ETH starts as 1:1 and slowly decreases over time with appreciation.

Copy link
Member Author

Choose a reason for hiding this comment

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

src/ynViewer.sol Outdated
_data[i] = StakingNodeData({
nodeId: _node.nodeId(),
ethBalance: _node.getETHBalance(),
eigenPodEthBalance: _eigenPod.nonBeaconChainETHBalanceWei(),
Copy link
Contributor

Choose a reason for hiding this comment

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

the meaning of this was intended to be:

address(_eigenPod).balance

Copy link
Member Author

Choose a reason for hiding this comment

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

src/ynViewer.sol Outdated
nodeId: _node.nodeId(),
ethBalance: _node.getETHBalance(),
eigenPodEthBalance: _eigenPod.nonBeaconChainETHBalanceWei(),
podOwnerShares: stakingNodesManager.strategyManager().stakerStrategyShares(address(_node), _node.beaconChainETHStrategy()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Here the intention was:

EigenPodManager.podOwnerShares()

It's a good question if those are in sync beaconChainETHStrategy. not 100% sure.

What we are using in accounting in the prod logic is

EigenPodManager.podOwnerShares()

Let's use that

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,28 @@
// SPDX-License-Identifier: BSD 3-Clause License
Copy link
Contributor

Choose a reason for hiding this comment

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

call the file

deployYnViewer.s.sol

Copy link
Member Author

Choose a reason for hiding this comment

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

privateKey == 0 ? vm.envUint("PRIVATE_KEY") : privateKey;
vm.startBroadcast(privateKey);

viewer = new ynViewer(_chainAddresses.yn.YNETH_ADDRESS, _chainAddresses.yn.STAKING_NODES_MANAGER_ADDRESS);
Copy link
Contributor

Choose a reason for hiding this comment

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

make it proxy upradeable, like the other ones - reason is if we need to update logic to keep this address constant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@danoctavian danoctavian left a comment

Choose a reason for hiding this comment

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

Requested some changes!

Good work overall

Copy link
Contributor

@danoctavian danoctavian left a comment

Choose a reason for hiding this comment

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

LGTM good work

@danoctavian danoctavian merged commit 8048c3c into main Jul 3, 2024
1 check passed
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