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.

Sign up for GitHub

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

Jump to bottom

feat(callbacks): return data in callbacks #190

Closed
wants to merge 9 commits into from
Closed

feat(callbacks): return data in callbacks #190

wants to merge 9 commits into from

Conversation

makcandrov
Copy link
Contributor

@makcandrov makcandrov commented Jul 27, 2023 •
edited
Loading

IMO:

  • (1) adds too much complexity
  • (2) is ok to implement

Question about (1): should we transfer the funds to the contract called or to msg.sender?

@makcandrov makcandrov changed the base branch from main to feat/callbacks July 27, 2023 08:26
MerlinEgalite
MerlinEgalite requested changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

Well I'm not sure I like it as well...

@makcandrov
Copy link
Contributor Author

Are you against (1) or (2)? Or both?
Imo (2) could be great and doesn't complicate too much the code

@MerlinEgalite
Copy link
Contributor

I'm against (1) and ok with (2) I think

@pakim249CAL
Copy link
Contributor

I think (1) is pretty overkill. It seems more appropriate for a user using callbacks to manage this kind of thing themselves by just calling the intended receiver in their own callback.
I also think (2) is not really necessary for the same reason.

Base automatically changed from feat/callbacks to main July 28, 2023 15:04
@makcandrov
Copy link
Contributor Author

I guess everyone agrees that (1) is not wanted

@makcandrov makcandrov changed the title feat(callbacks): enhance callbacks feat(callbacks): return data in callbacks Jul 28, 2023
@makcandrov makcandrov marked this pull request as ready for review July 28, 2023 15:45
MerlinEgalite
MerlinEgalite requested changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

well seeing the code I think I'm against

Rubilmax
Rubilmax previously approved these changes Jul 31, 2023
View reviewed changes
MerlinEgalite
MerlinEgalite requested changes Aug 2, 2023
View reviewed changes
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

I'm still against tbh

@makcandrov
Copy link
Contributor Author

makcandrov commented Aug 2, 2023 •
edited
Loading

yes I'm just keeping it up to date until we decide to close it I guess

@makcandrov
Copy link
Contributor Author

I highly prefer returning the amounts/shares (#215 and/or #181). This PR doesn't seem compatible with them, so I'm in favor of closing it.

@Rubilmax Rubilmax mentioned this pull request Aug 9, 2023
Closed
@Rubilmax Rubilmax mentioned this pull request Oct 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MerlinEgalite MerlinEgalite MerlinEgalite requested changes

@Rubilmax Rubilmax Rubilmax approved these changes

@MathisGD MathisGD Awaiting requested review from MathisGD

@pakim249CAL pakim249CAL Awaiting requested review from pakim249CAL

@Jean-Grimal Jean-Grimal Awaiting requested review from Jean-Grimal

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@makcandrov @MerlinEgalite @pakim249CAL @Rubilmax