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

CIP [0014]: Add new exchange function to allow buying fixed amount of tokens for maximum price #26

Closed
wants to merge 2 commits into from

Conversation

zviadm
Copy link
Contributor

@zviadm zviadm commented Jun 9, 2020

No description provided.

@zviadm zviadm changed the title [0014]: Add new exchange function to allow buying fixed amount of tokens for maximum price CIP [0014]: Add new exchange function to allow buying fixed amount of tokens for maximum price Jun 17, 2020
Copy link

@asaj asaj left a comment

Choose a reason for hiding this comment

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

This seems like a great idea and should be pretty straightforward to implement!

@asaj
Copy link

asaj commented Jul 27, 2020

Wdyt of the following function signatures, and deprecating the exchange function?

function buy(uint256 amount, uint256 minSellAmount, bool buyGold);
function sell(uint256 amount, uint256 minBuyAmount, bool sellGold);

cc @tkporter

@zviadm
Copy link
Contributor Author

zviadm commented Jul 27, 2020

This works, and it should cover all the use cases that I mentioned in the CIP.

@tkporter
Copy link
Contributor

tkporter commented Jul 30, 2020

@asaj PR will be out soon

Shouldn't buy enforce a maxSellAmount instead of minSellAmount?

function buy(uint256 amount, uint256 maxSellAmount, bool buyGold);

edit: ah I see now the CIP says this too

@tkporter
Copy link
Contributor

PR is up at celo-org/celo-monorepo#4580

mergify bot pushed a commit to celo-org/celo-monorepo that referenced this pull request Aug 2, 2020
…#4580)

### Description

* Adds `function buy(uint256 buyAmount, uint256 maxSellAmount, bool buyGold)`
* Adds `function sell(uint256 sellAmount, uint256 minBuyAmount, bool sellGold)`
* Deprecates `function exchange(uint256 sellAmount, uint256 minBuyAmount, bool sellGold)` which has the same functionality as `sell`
* Runs same suite of tests for `sell` and `exchange`
* Adds tests for `buy`

### Other changes

n/a

### Tested

Ran tests

### Related issues

Related CIP: celo-org/celo-proposals#26

### Backwards compatibility

Fully compatible
@martinvol
Copy link
Contributor

Shall we close as celo-org/celo-monorepo#4580 got merged?

@nambrot
Copy link
Contributor

nambrot commented Sep 23, 2020

Agreed @tkporter ?

@prestwich
Copy link
Contributor

Closing as obsolete

@prestwich prestwich closed this Dec 7, 2020
@tkporter
Copy link
Contributor

tkporter commented Dec 7, 2020

Whoops didn't see this-- is there a reason why we'd close instead of merge?

@prestwich
Copy link
Contributor

hey @tkporter trying to clean up stale PRs a bit. I asked Nam's opinion but probably ought to have pinged you as well. If this accurately documents current behavior we should clean it up and include it

@tkporter
Copy link
Contributor

tkporter commented Dec 7, 2020

Yeah this was implemented in celo-org/celo-monorepo#4580. For cleaning up, would that involve making some changes to fit the template https://github.com/celo-org/celo-proposals/blob/master/CIPs/cip-template.md?

@prestwich
Copy link
Contributor

For cleaning up, would that involve making some changes to fit the template https://github.com/celo-org/celo-proposals/blob/master/CIPs/cip-template.md?

Yes, assuming it's already implemented, this should be

category: Ring 1
type: Standards Track
status: Final

pedro-clabs added a commit that referenced this pull request Apr 13, 2021
YazzyYaz pushed a commit that referenced this pull request Apr 14, 2021
* Mark CGP #24 as proposed (proposal ID #26).

* Add @pedro-clabs to CGP #24 authors list.
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.

6 participants