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

Dutch auction #78

Closed
wants to merge 164 commits into from
Closed

Dutch auction #78

wants to merge 164 commits into from

Conversation

SwayStar123
Copy link
Member

@SwayStar123 SwayStar123 commented Jun 8, 2022

Summary

Type of change

  • New Feature

Changes

  • Adding a new dutch auction program

Related Issues

Closes #69

@SwayStar123 SwayStar123 added the New Application Application specification label Jun 8, 2022
@Braqzen Braqzen added New Feature New addition that does not currently exist and removed New Application Application specification labels Jun 8, 2022
Copy link
Contributor

@Braqzen Braqzen left a comment

Choose a reason for hiding this comment

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

This shouldn't be at the top level. Move it all to sway-applications/auctions/dutch-acution
Add a README.
Change "type of change" to "New Feature" instead of your custom "New Program" and add a space after "-" so that it looks like a bullet point.
Same with "Changes" for the bullet point and be more descriptive there.
You've linked to the issue, no need to copy and paste the same content into the "additional" section. remove that entirely.
Create a logo and insert it into the readme too.

I've probably made some mistakes / not reviewed it to the same extent that I would otherwise because of how messy this is. It's difficult to keep everything in mind. I will need to make additional reviews after changes are made.

Oh, you must use forc build to check your work and also forc fmt.
I've forgotten to mention but also this needs to be added to the CI in .github/workflows and the script too

dutch-auction/Cargo.toml Outdated Show resolved Hide resolved
dutch-auction/Cargo.toml Outdated Show resolved Hide resolved
dutch-auction/Forc.toml Outdated Show resolved Hide resolved
dutch-auction/src/main.sw Outdated Show resolved Hide resolved
dutch-auction/src/main.sw Outdated Show resolved Hide resolved
dutch-auction/tests/harness.rs Outdated Show resolved Hide resolved
dutch-auction/src/main.sw Outdated Show resolved Hide resolved
dutch-auction/src/main.sw Outdated Show resolved Hide resolved
dutch-auction/src/main.sw Outdated Show resolved Hide resolved
dutch-auction/src/main.sw Outdated Show resolved Hide resolved
@adlerjohn adlerjohn changed the title 69 dutch auction Dutch auction Jun 8, 2022
@SwayStar123
Copy link
Member Author

If everything looks good ill move on to the specification and readme

Comment on lines 43 to +44
"auctions/english-auction",
"auctions/dutch-auction",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"auctions/english-auction",
"auctions/dutch-auction",
"auctions/dutch-auction",
"auctions/english-auction",

@@ -0,0 +1,9 @@
# Overview
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest looking at the other README files and adjusting this one to contain a higher level structure / scope / content of the auctions as this only has a few lines / bullet points. It's too barebones.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would check the repo to see if this is even needed or if a repo level gitignore already contains there

license = "Apache-2.0"

[dependencies]
fuels = { version = "0.38.1", features = ["fuel-core-lib"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

I would bump all versions to be 0.41.0

@@ -0,0 +1,9 @@
# Overview
Copy link
Contributor

Choose a reason for hiding this comment

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

I would model the english auction docs for this file


// retrieving active_auctions_of_author and auctions_of_author and auction_count before
let auction_count_before = auction_count(&instance).await;
let active_auctions_of_author_before =
Copy link
Contributor

Choose a reason for hiding this comment

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

This creation of an identity is being used repeatedly, I would wrap it into a function

@@ -0,0 +1,11 @@
pub mod active_auctions_of_author;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these need to be pub?

Comment on lines +1 to +3
pub mod utils;

pub mod functions;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub mod utils;
pub mod functions;
pub mod functions;
pub mod utils;

@@ -0,0 +1,177 @@
use fuels::{prelude::*, types::Identity};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd explicitly import

(DutchAuction::new(id, wallet.clone()), wallet)
}

pub async fn active_auctions_of_author(instance: &DutchAuction, author: Identity) -> Vec<u64> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd create a setup.rs for the setup and an interface dir which splits the content into core / info functionality. Take a look at the other apps for examples of how to do this

@bitzoic
Copy link
Member

bitzoic commented Oct 5, 2023

Is this still being developed?

@SwayStar123
Copy link
Member Author

Is this still being developed?

Not by me, someone can take over if they want

@SwayStar123
Copy link
Member Author

Closed as this is no longer being worked on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App: Auctions Label used to filter for the app issue Awaiting SDK feature(s) SDK does not support desired functionality, yet New Feature New addition that does not currently exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dutch auction program
4 participants