-
Notifications
You must be signed in to change notification settings - Fork 364
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
feat: Initial implementation of auction dApp #2265
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auctions are a common thing in other web3 ecosystems, so thanks for adding this -- it's a good start.
What is the intended way to have users interact with auctions? I see that the realm returns an instance of the auction object to the caller. This may not be desirable because the person receiving the auction object could call the AddBid
method, circumventing the the checks inside the the realm's AddBid
function.
It seems to me like the realm would be the container for all auction objects and the auction specific logic could live within the package, so the error checking logic in the realm's AddBid
function could be moved to the package, unless the intention is to give realms using the package full control over how these checks should look, though some of them seem pretty universal.
Also, I left a comment regarding the auction structs' fields being unexported. Exporting these fields would remove the need for all of the accessor methods, as well as let the realms using the package decide whether or not they want to let auction owners adjust auction details after they've been created. I could be persuaded either way; just something to consider.
|
||
// Main struct | ||
type Auction struct { | ||
title string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why the fields for these structs can't be exported so we can remove all of the getter methods? If auctions are made to be self contained within the realm itself (don't return any references to auctions), then it should be okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really sorry for the long delay in replying. 6936b8
} | ||
|
||
// NewBid creates a new Bid instance. | ||
func NewBid(bidder std.Address, amount uint64) *Bid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to get rid of this function -- not calling it will save gas and it's only used in one spot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the NewBid()
function and updated the AddBid()
and the IsOwnedBy()
functions 5e47aa9.
panic("Auction cannot end before the end time") | ||
} | ||
if a.bids.Size() == 0 { | ||
panic("No bids placed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? If the end time has passed for an auction then the realm prevents adding a bid, but this prevents marking the auction as ended, so the auction can never end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced panic
by ufmt.Errorf
which handles other error cases dd5b4911
begin time.Time | ||
end time.Time | ||
price uint64 | ||
bids *avl.Tree // key: std.Address, value: Bid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is wrong; the key being used when adding new bids is a integer string.
Even so, why use an avl tree to represent bids instead of something like a slice? The bid struct contains the bidder's address already, so no need to use it as a key. Using a integer as a key is kind of like using a slice where the integer is the key by default.
} | ||
|
||
// Mockable function to get current time | ||
func now() time.Time { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to keep testing logic in the testing file. There is a pending PR that will allow you to change the timestamp from your test code; it would be good to use that when it is merged #569
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, please drop currentTime
} | ||
|
||
// renderHomepage renders the homepage of the realm | ||
func renderHomepage() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rendering all auctions with all bids is a lot for one page. Consider having the homepage render links to each auction that, when rendered, display all the auction bids.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i updated the auction
realm and i imported the mux
package and added render
pages for each auction state 9d634721
Hi @deelawn, Thank you for the detailed feedback! I appreciate your insights and suggestions. I understand the possibility of users bypassing the controls inside the realm's
I am currently working on implementing these changes and will push an update shortly. Thank you for the constructive feedback and helping :) |
Co-authored-by: deelawn <dboltz03@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @deelawn,
Thanks again for your insights,I wanted to let you know that I plan to integrate accesscontrol and timelock to give the bidders time to withdraw if there is something fishy functionalities into the logic of the auction.
Please let me know if you have any further suggestions or comments.
@mous1985 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 💯
The tests could use a bit more work, I think proptests would fit in nicely here.
Please see @deelawn's comments -- I've also left a couple 🙏
"gno.land/p/demo/avl" | ||
) | ||
|
||
// Main struct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no needs for comments like these :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for the delay in replying 🙏.I deleted the comment 😅 ,and replaced avlTree with a simple slice 6936b8
title string, | ||
owner std.Address, | ||
description string, | ||
begin time.Time, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about having begin
actually be time.Now
?
This way you can decrease the param. It's only assuming you don't have auctions that start in the future, given that you emit an event when this constructor exits
end
can be time.Duration
if this is your preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the package with three auction states: “upcoming”, “ongoing”, and “closed”. That was my idea, so I could have auctions starting in the future.
// GetBids returns an array of all the bids made in the auction. | ||
func (a Auction) GetBids() []*Bid { | ||
bids := make([]*Bid, 0, a.bids.Size()) | ||
a.bids.Iterate("", "", func(key string, value interface{}) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding some kind of pagination here, to circumvent an execution attack if there are many bids?
} | ||
|
||
// IsOwner checks if the given address is the owner of the auction. | ||
func (a Auction) IsOwnedBy(address std.Address) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use Ownable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if !ok { | ||
panic("auction does not exist") | ||
} | ||
auction := auc.(*auctionp.Auction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can delegate these checks to the package AddBid
instead
// renderHomepage renders the homepage of the realm | ||
func renderHomepage() string { | ||
var b bytes.Buffer | ||
b.WriteString("<h1><center>Auctions</center></h1>\n\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not a simple #?
return b.String() | ||
} | ||
|
||
// Helper function to set the mock current time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need these
begin := now().Add(1 * time.Second).Unix() // Auction begins in 1 second | ||
end := now().Add(24 * time.Hour).Unix() // Auction ends in 24 hours | ||
auc := NewAuction("Test Auction", "A simple test auction", begin, end, 100) | ||
if auc == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use uassert here
setcurrentTime(now().Add(2 * time.Second)) | ||
|
||
// Place bids by different users | ||
user1 := std.Address("user1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use testutils
for generating an address, not casting it directly like this
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2265 +/- ##
==========================================
- Coverage 60.23% 60.20% -0.03%
==========================================
Files 562 562
Lines 75091 75038 -53
==========================================
- Hits 45230 45178 -52
- Misses 26481 26483 +2
+ Partials 3380 3377 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@mous1985, What is the status on this? Can you check and address the comments? |
replace the auction liste avlTree by slice
update AddBid update isOwnedBy
Initial Implementation of dAuction
Purpose
This pull request introduces the initial version (v0) of the auction dApp. The dApp allows users to create and manage auctions. It is designed to interact with an existing NFT realm to handle the state and ownership of NFTs, with future plans to integrate NFTs directly into the auction struct and automate transfers upon auction completion.
Main Features
State Management: The auction state is managed by the realm, ensuring persistence and consistency across transactions.
How It Works
p/demo/auction
:The auction package handles the core logic for creating and * managing auctions.
It defines the Auction and Bid structs and provides methods for creating auctions, adding bids, and ending auctions.
Future plans include integrating NFTs into the Auction struct to directly manage NFT auctions.
r/demo/auction
:The auction realm maintains the state of all auctions using an AVL tree.
It uses the auction package to handle auction operations and interacts with the NFT realm to manage NFT state and transfers.
The realm exposes methods for creating auctions, placing bids, and rendering the current state.
Testing:
The auction_test.gno file contains test cases that simulate auction creation, bidding, and ending.
Tests ensure that the core functionalities work as expected, including state changes and interactions with the NFT realm.
Future Enhancements
Integrate NFTs: Future updates will include adding an nft.TokenID field to the Auction struct and enhancing the logic to support direct NFT auctions.
Deposits and Automatic Transfers:
Additional Features: Plan to add more features such as auction cancellation, bid retraction, and more detailed auction statistics .
PS: This pull request lays the foundation for the auction dApp, and future enhancements can build on this initial implementation to add more features and improve functionality. Feel free to contribute and share any ideas :) .
@moul @leohhhn
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description