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

modify: refactor code #473

Merged
merged 2 commits into from
Apr 3, 2023
+718 −256
Merged

modify: refactor code #473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ef34b3
modify: refactor code
mkXultra Apr 3, 2023
c936ff6
modify: refactor code
mkXultra Apr 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions x/nftmarket/keeper/bid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,19 +345,20 @@ func (k Keeper) SafeCloseBid(ctx sdk.Context, bid types.NftBid) error {
return k.ManualSafeCloseBid(ctx, bid, bidder)
}

func (k Keeper) SafeCloseBidCollectDeposit(ctx sdk.Context, bid types.NftBid, listing types.NftListing) (types.NftListing, error) {
listing.CollectedAmount = listing.CollectedAmount.Add(bid.DepositAmount)
func (k Keeper) SafeCloseBidCollectDeposit(ctx sdk.Context, bid types.NftBid) (sdk.Coin, error) {
CollectedAmount := bid.DepositAmount
k.DeleteBid(ctx, bid)
return listing, nil
return CollectedAmount, nil
}

// todo make unit test
func (k Keeper) SafeCloseBidWithAllInterest(ctx sdk.Context, bid types.NftBid) error {
bidder, err := sdk.AccAddressFromBech32(bid.Bidder)
if err != nil {
return err
}
interestAmount := bid.TotalInterestAmount(ctx.BlockTime())
if interestAmount.Amount.GT(sdk.ZeroInt()) {
if interestAmount.Amount.IsPositive() {
err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, bidder, sdk.Coins{sdk.NewCoin(interestAmount.Denom, interestAmount.Amount)})
if err != nil {
return err
Copy link

Choose a reason for hiding this comment

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

the code review for this patch

  1. The patch looks good, however it would be better to add a comment before the new added lines (todo make unit test) to explain the purpose of that line.
  2. The function SafeCloseBidCollectDeposit has been changed to return CollectedAmount instead of listing. It would be nice to add a comment to explain why the change was made.
  3. The if condition in SafeCloseBidWithAllInterest has been changed from GT(sdk.ZeroInt()) to IsPositive(). It would be better to add a comment to explain why the change was made.
  4. The patch should include unit tests to ensure the changes are working as expected.

Copy link

Choose a reason for hiding this comment

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

The code review for the given patch is as follows:

  • In the SafeCloseBidCollectDeposit function, there seems to be a mistake in updating the CollectedAmount field of the listing parameter. Instead of updating the parameter itself, it should create a new copy of the parameter and update its CollectedAmount field.
  • The return type of the SafeCloseBidCollectDeposit function should be changed from types.NftListing to sdk.Coin, since only the collected amount needs to be returned.
  • The SafeCloseBidWithAllInterest function has a TODO comment to add unit tests, which should be addressed.
  • Instead of checking if interestAmount.Amount is greater than zero, use the IsPositive() method provided by the SDK.
  • Overall, the code looks good with no obvious bug risks. However, since we do not have the full context of the system, there might be some issues specific to this codebase that could not be captured here. Therefore, careful testing and review of the entire system are still necessary.

Expand Down
244 changes: 159 additions & 85 deletions x/nftmarket/keeper/bid_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,95 +13,169 @@ import (
"github.com/UnUniFi/chain/x/nftmarket/types"
)

// test basic functions of bids on nft bids
// func (suite *KeeperTestSuite) TestNftBidBasics() {
// owner := sdk.AccAddress(ed25519.GenPrivKey().PubKey().Address().Bytes())
// owner2 := sdk.AccAddress(ed25519.GenPrivKey().PubKey().Address().Bytes())

// now := time.Now().UTC()
// bids := []types.NftBid{
// {
// NftId: types.NftIdentifier{
// ClassId: "1",
// NftId: "1",
// },
// Bidder: owner.String(),
// BidAmount: sdk.NewInt64Coin("uguu", 1000000),
// AutomaticPayment: true,
// DepositAmount: sdk.NewInt64Coin("uguu", 1000000),
// BidTime: now,
// },
// {
// NftId: types.NftIdentifier{
// ClassId: "1",
// NftId: "1",
// },
// Bidder: owner2.String(),
// BidAmount: sdk.NewInt64Coin("uguu", 1000000),
// AutomaticPayment: true,
// DepositAmount: sdk.NewInt64Coin("uguu", 1000000),
// BidTime: now,
// },
// {
// NftId: types.NftIdentifier{
// ClassId: "1",
// NftId: "2",
// },
// Bidder: owner.String(),
// BidAmount: sdk.NewInt64Coin("uguu", 1000000),
// AutomaticPayment: true,
// DepositAmount: sdk.NewInt64Coin("uguu", 1000000),
// BidTime: now,
// },
// }

// for _, bid := range bids {
// suite.app.NftmarketKeeper.SetBid(suite.ctx, bid)
// }

// for _, bid := range bids {
// bidder, err := sdk.AccAddressFromBech32(bid.Bidder)
// suite.Require().NoError(err)
// gotBid, err := suite.app.NftmarketKeeper.GetBid(suite.ctx, bid.NftId.IdBytes(), bidder)
// suite.Require().NoError(err)
// suite.Require().Equal(bid, gotBid)
// }

// // check all bids
// allBids := suite.app.NftmarketKeeper.GetAllBids(suite.ctx)
// suite.Require().Len(allBids, len(bids))

// // check bids by bidder
// bidsByOwner := suite.app.NftmarketKeeper.GetBidsByBidder(suite.ctx, owner)
// suite.Require().Len(bidsByOwner, 2)

// // check bids by nft
// nftBids := suite.app.NftmarketKeeper.GetBidsByNft(suite.ctx, (types.NftIdentifier{
// ClassId: "1",
// NftId: "1",
// }).IdBytes())
// suite.Require().Len(nftBids, 2)
func (suite *KeeperTestSuite) TestNftBidBasics() {
owner := sdk.AccAddress(ed25519.GenPrivKey().PubKey().Address().Bytes())
bidder := sdk.AccAddress(ed25519.GenPrivKey().PubKey().Address().Bytes())
normalListing := types.NftListing{
NftId: types.NftIdentifier{
ClassId: "class1",
NftId: "nft1",
},
Owner: owner.String(),
ListingType: types.ListingType_DIRECT_ASSET_BORROW,
BidToken: "uguu",
MinimumDepositRate: sdk.MustNewDecFromStr("0.1"),
AutomaticRefinancing: true,
MinimumBiddingPeriod: time.Hour * 1,
State: types.ListingState_LISTING,
}
endListing := types.NftListing{
NftId: types.NftIdentifier{
ClassId: "class2",
NftId: "nft2",
},
Owner: owner.String(),
ListingType: types.ListingType_DIRECT_ASSET_BORROW,
BidToken: "uguu",
MinimumDepositRate: sdk.MustNewDecFromStr("0.1"),
AutomaticRefinancing: true,
MinimumBiddingPeriod: time.Hour * 1,
State: types.ListingState_END_LISTING,
}
biddingListing := types.NftListing{
NftId: types.NftIdentifier{
ClassId: "class3",
NftId: "nft3",
},
Owner: owner.String(),
ListingType: types.ListingType_DIRECT_ASSET_BORROW,
BidToken: "uguu",
MinimumDepositRate: sdk.MustNewDecFromStr("0.1"),
AutomaticRefinancing: true,
MinimumBiddingPeriod: time.Hour * 1,
State: types.ListingState_BIDDING,
}
testCases := []struct {
testCase string
bid types.MsgPlaceBid
listing *types.NftListing
expErr error
}{
{
testCase: "test basic functions of bids on nft bids1",
bid: types.MsgPlaceBid{
Sender: bidder.Bytes(),
NftId: normalListing.NftId,
BidAmount: sdk.NewInt64Coin("uguu", 1000000),
DepositAmount: sdk.NewInt64Coin("uguu", 1000000/2),
AutomaticPayment: true,
BiddingPeriod: time.Now().Add(time.Hour * 2),
},
listing: &normalListing,
expErr: nil,
},
{
testCase: "test basic functions of bids on nft bids2",
bid: types.MsgPlaceBid{
Sender: bidder.Bytes(),
NftId: biddingListing.NftId,
BidAmount: sdk.NewInt64Coin("uguu", 1000000),
DepositAmount: sdk.NewInt64Coin("uguu", 1000000/2),
AutomaticPayment: true,
BiddingPeriod: time.Now().Add(time.Hour * 2),
},
listing: &biddingListing,
expErr: nil,
},
{
testCase: "not exist listing",
bid: types.MsgPlaceBid{
Sender: bidder.Bytes(),
NftId: normalListing.NftId,
BidAmount: sdk.NewInt64Coin("uguu", 1000000),
DepositAmount: sdk.NewInt64Coin("uguu", 1000000/2),
AutomaticPayment: true,
BiddingPeriod: time.Now().Add(time.Hour * 2),
},
listing: nil,
expErr: fmt.Errorf("nft listing does not exist"),
},
{
testCase: "can not bid",
bid: types.MsgPlaceBid{
Sender: bidder.Bytes(),
NftId: endListing.NftId,
BidAmount: sdk.NewInt64Coin("uguu", 1000000),
DepositAmount: sdk.NewInt64Coin("uguu", 1000000/2),
AutomaticPayment: true,
BiddingPeriod: time.Now().Add(time.Hour * 2),
},
listing: &endListing,
expErr: types.ErrNftListingNotInBidState,
},
{
testCase: "invalid bid token",
bid: types.MsgPlaceBid{
Sender: bidder.Bytes(),
NftId: normalListing.NftId,
BidAmount: sdk.NewInt64Coin("xxx", 1000000),
DepositAmount: sdk.NewInt64Coin("xxx", 1000000/2),
AutomaticPayment: true,
BiddingPeriod: time.Now().Add(time.Hour * 2),
},
listing: &normalListing,
expErr: types.ErrInvalidBidDenom,
},
{
testCase: "not enough bidding period",
bid: types.MsgPlaceBid{
Sender: bidder.Bytes(),
NftId: normalListing.NftId,
BidAmount: sdk.NewInt64Coin("uguu", 1000000),
DepositAmount: sdk.NewInt64Coin("uguu", 1000000/2),
AutomaticPayment: true,
BiddingPeriod: time.Now().Add(time.Minute * 1),
},
listing: &normalListing,
expErr: types.ErrSmallBiddingPeriod,
},
}
for _, tc := range testCases {
var err error
initUguuAmount := sdk.NewInt64Coin("uguu", 100000000)
initAmount := sdk.NewCoins(initUguuAmount)
err = suite.app.BankKeeper.MintCoins(suite.ctx, minttypes.ModuleName, initAmount)
suite.Require().NoError(err, tc.testCase)
err = suite.app.BankKeeper.SendCoinsFromModuleToAccount(suite.ctx, minttypes.ModuleName,
tc.bid.Sender.AccAddress(), initAmount)
suite.Require().NoError(err, tc.testCase)
if tc.listing != nil {
suite.keeper.SetNftListing(suite.ctx, *tc.listing)
}

// // delete all the bids
// for _, bid := range bids {
// suite.app.NftmarketKeeper.DeleteBid(suite.ctx, bid)
// }
err = suite.keeper.PlaceBid(suite.ctx, &tc.bid)
if tc.expErr != nil {
suite.Require().Error(err, tc.testCase)
suite.Require().Equal(tc.expErr.Error(), err.Error(), tc.testCase)
continue
}

// // check all bids
// allBids = suite.app.NftmarketKeeper.GetAllBids(suite.ctx)
// suite.Require().Len(allBids, 0)
suite.Require().NoError(err, tc.testCase)
afterAmount := suite.app.BankKeeper.GetBalance(suite.ctx, tc.bid.Sender.AccAddress(), "uguu")
suite.Equal(afterAmount.Add(tc.bid.DepositAmount), initUguuAmount, tc.testCase)

// // check bids by bidder
// bidsByOwner = suite.app.NftmarketKeeper.GetBidsByBidder(suite.ctx, owner)
// suite.Require().Len(bidsByOwner, 0)
// cleanup
suite.keeper.DeleteNftListing(suite.ctx, *tc.listing)
err = suite.app.BankKeeper.SendCoinsFromAccountToModule(suite.ctx, tc.bid.Sender.AccAddress(), types.ModuleName, sdk.NewCoins(afterAmount))
suite.Require().NoError(err, tc.testCase)
cleanupAmount := suite.app.BankKeeper.GetBalance(suite.ctx, tc.bid.Sender.AccAddress(), "uguu")
suite.Equal(cleanupAmount, sdk.NewCoin("uguu", sdk.ZeroInt()), tc.testCase)
}
}

// // check bids by nft
// nftBids = suite.app.NftmarketKeeper.GetBidsByNft(suite.ctx, (types.NftIdentifier{
// ClassId: "1",
// NftId: "1",
// }).IdBytes())
// suite.Require().Len(nftBids, 0)
// }
//todo make Rebid tests
//todo make FirstBid tests
//todo make ManualBid tests

func (suite *KeeperTestSuite) TestCancelledBid() {
owner := sdk.AccAddress(ed25519.GenPrivKey().PubKey().Address().Bytes())
Expand Down
Loading