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

add compilation restrictions #402

Merged
merged 22 commits into from
Dec 9, 2024
Merged

add compilation restrictions #402

merged 22 commits into from
Dec 9, 2024

Conversation

gretzke
Copy link
Contributor

@gretzke gretzke commented Nov 26, 2024

No description provided.

@dianakocsis
Copy link
Contributor

could be removed

import {IPositionManager} from "../../src/interfaces/IPositionManager.sol";

@dianakocsis
Copy link
Contributor

getting warnings on these:

function notifyBurn(uint256 tokenId, address owner, PositionInfo info, uint256 liquidity, BalanceDelta feesAccrued)

function notifyBurn(uint256 tokenId, address owner, PositionInfo info, uint256 liquidity, BalanceDelta feesAccrued)

function notifyBurn(uint256 tokenId, address owner, PositionInfo info, uint256 liquidity, BalanceDelta feesAccrued)

foundry.toml Outdated Show resolved Hide resolved
test/shared/Deploy.sol Outdated Show resolved Hide resolved
@snreynolds snreynolds mentioned this pull request Dec 3, 2024
test/shared/Deploy.sol Outdated Show resolved Hide resolved
Copy link
Collaborator

@saucepoint saucepoint left a comment

Choose a reason for hiding this comment

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

looks pretty good to me; did NOT look at PositionDescriptor.t.sol btw 😅

script/DeployStateView.s.sol Outdated Show resolved Hide resolved
script/DeployV4Quoter.s.sol Outdated Show resolved Hide resolved
src/interfaces/IPositionDescriptor.sol Show resolved Hide resolved
src/interfaces/IPoolInitializer.sol Outdated Show resolved Hide resolved
src/interfaces/IStateView.sol Outdated Show resolved Hide resolved

import {IAllowanceTransfer} from "permit2/src/interfaces/IAllowanceTransfer.sol";

/// @notice Permit2Forwarder allows permitting this contract as a spender on permit2
Copy link
Contributor

@dianakocsis dianakocsis Dec 9, 2024

Choose a reason for hiding this comment

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

this is the exact same natspec as Permit2Forwarder. I feel like the top of interfaces should all be the same, maybe like how it is for IPositionManager.sol.

/// @title IPermit2Forwarder
/// @notice Interface for the Permit2Forwarder contract

and then we can follow this standard across all of our interfaces, and then the natspec at the top of the contracts could go in more detail?

Copy link
Member

Choose a reason for hiding this comment

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

sure!

@snreynolds snreynolds merged commit ab76d3b into main Dec 9, 2024
3 checks passed
@snreynolds snreynolds deleted the compilation-restrictions branch December 9, 2024 21:43
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