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

Does not expose view #313

Open
Yorkemartin opened this issue Jul 19, 2021 · 6 comments
Open

Does not expose view #313

Yorkemartin opened this issue Jul 19, 2021 · 6 comments

Comments

@Yorkemartin
Copy link

would be nice to return both view and override - for the moment they still require manual addition afterward.

@frangio
Copy link
Contributor

frangio commented Jul 20, 2021

What information about override do you think should be exposed? Just that a function is an override?

@Yorkemartin
Copy link
Author

if a function is marked as override - this implicitly changes the functionality of external functions.

i think override and view should be exposed - both are relevant to developers understanding external functions

@frangio
Copy link
Contributor

frangio commented Jul 20, 2021

I agree that view is relevant in API docs. But I don't understand why override would be included, since I see it as an implementation detail.

@Yorkemartin
Copy link
Author

an example:

In Uniswap V3 - users may call a variety of external contracts which are tailored for different objectives. In one case, one will call the Router contract to execute a swap, but another may call a Quoter contract to simulate the execution of a swap and return some kind of data.

Both of these contracts, the router and the quote, use this same callback function, which is called by the core contract towards the end of the original function.

function uniswapV3SwapCallback( int256 amount0Delta, int256 amount1Delta, bytes memory path ) external view override {

It seems odd to hide this functionality - we are exposing an external function that is callable by the core contracts - but hiding that the function is marked override, which seems misleading and may confuse integrators by leading them to believe the two instances of uniswapV3SwapCallback are the same

@frangio
Copy link
Contributor

frangio commented Jul 22, 2021

Can you show the pieces of documentation that are affected by this? I remain of the opinion that override is just an implementation detail, and that if there is any possibility to mislead, it should be documented in prose.

@Yorkemartin
Copy link
Author

im seeing your point now - i think just doing view is fine

@frangio frangio changed the title does not parse override Does not expose view Aug 9, 2021
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

No branches or pull requests

2 participants