feat(x/ecocredit/marketplace): implement buyer and seller fees #2154

aaronc · 2024-01-31T18:31:19Z

Description

Implementation for #2151

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

…onc/marketplace-fees

…onc/marketplace-fees-impl

codecov · 2024-01-31T20:47:32Z

Codecov Report

Attention: Patch coverage is 67.33668% with 65 lines in your changes are missing coverage. Please review.

Project coverage is 72.92%. Comparing base (3b21ecd) to head (dbf4e14).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2154      +/-   ##
==========================================
+ Coverage   72.90%   72.92%   +0.01%     
==========================================
  Files         240      243       +3     
  Lines       13860    14022     +162     
==========================================
+ Hits        10105    10225     +120     
- Misses       3016     3042      +26     
- Partials      739      755      +16

Files	Coverage Δ
app/app.go	`92.71% <100.00%> (+0.01%)`	⬆️
x/ecocredit/marketplace/keeper/keeper.go	`100.00% <100.00%> (ø)`
x/ecocredit/marketplace/types/v1/msg_buy_direct.go	`98.30% <100.00%> (+0.15%)`	⬆️
...ecocredit/marketplace/types/v1/state_fee_params.go	`100.00% <100.00%> (ø)`
x/ecocredit/server/tests/utils.go	`100.00% <100.00%> (ø)`
...dit/marketplace/types/v1/msg_gov_set_fee_params.go	`62.50% <64.28%> (+62.50%)`	⬆️
x/ecocredit/marketplace/keeper/msg_buy_direct.go	`79.31% <79.31%> (+1.18%)`	⬆️
...t/marketplace/keeper/msg_gov_send_from_fee_pool.go	`64.70% <64.70%> (ø)`
...marketplace/types/v1/msg_gov_send_from_fee_pool.go	`68.42% <68.42%> (ø)`
...redit/marketplace/keeper/msg_gov_set_fee_params.go	`50.00% <50.00%> (+50.00%)`	⬆️
... and 1 more

…onc/marketplace-fees-impl

aaronc · 2024-02-05T19:54:46Z

x/ecocredit/server/server.go

@@ -67,7 +67,7 @@ func NewServer(storeKey storetypes.StoreKey,
 	s.marketplaceStore = marketStore
 	s.BaseKeeper = basekeeper.NewKeeper(baseStore, bankKeeper, baseAddr, basketStore, marketStore, authority)
 	s.BasketKeeper = basketkeeper.NewKeeper(basketStore, baseStore, bankKeeper, basketAddr, authority)
-	s.MarketplaceKeeper = marketkeeper.NewKeeper(marketStore, baseStore, bankKeeper, authority)
+	s.MarketplaceKeeper = marketkeeper.NewKeeper(marketStore, baseStore, bankKeeper, authority, ecocredit.ModuleName)


Question for reviewers: Should the fee pool be the main ecocredit module account or should it be separated into a separate module account? How do we give governance the ability to govern over this fee pool (likely in a separate PR)?

I believe it should be its own account, it will also make it easier if it evolves into its own module.

@JeancarloBarrios see 3376e76 and caa606c where:

a marketplace fee pool module account is set up,

and I added a new MsgGovSendFromFeePool message so that governance can actually manage the fee pool funds

Let me know if this all looks good

yeah It looks good to me

proto/regen/ecocredit/marketplace/v1/tx.proto

paul121 · 2024-02-14T15:43:18Z

x/ecocredit/marketplace/keeper/features/msg_buy_direct.feature

+      * expect bob bank balance "0foo"
+      * expect fee pool balance "15foo"
+
+    Scenario: fees get burned


Is an event emitted when fees are burned? I see above there are scenarios to test that specific events are emitted. Maybe that isn't needed here since it would not be an event type provided by marketplace submodule.

We can emit the burn event here. Don't have a strong preference.

It seems like a burn event should be emitted but it just isn't clear to me if that is already happening. Does the bank keeper do that for us? If so, then maybe we don't need to include in tests.

The bank keeper does include burn events, but since MsgBuyDirect now also includes the fees paid and we can assume that if the denom is uregen that it was a burn, is it okay if we just leave it at that and not add another event?

What are the pros and cons of adding a specific event here? seems like for users, it is useful to have a specific event.

I don't know if there's really a pro to adding it. The con is that it just adds duplicitous events which takes up extra storage. The bank keeper and marketplace module already emit events containing the relevant data, so I think we should leave it as is.

Sorry I might have created some confusion here. I just wanted to make sure that a burn event does exist. I agree we do not need a duplicate event. This would all be very clear if the test scenario included checking that the burn event is emitted but no worries if this is non-trivial to add.

paul121 · 2024-02-14T16:20:39Z

x/ecocredit/marketplace/keeper/utils.go

+		return err
+	}
+
+	// if total fee > 0, then transfer total fee from buyer account to fee pool


This looks good to me, just a question on how the fees/costs are moving around.

As implemented here a successful MsgBuyDirect in a TX with quantity 10, cost 10uregen and both a buyer fee 0.1 and seller fee 0.05, will result in two separate transfer events in the message :

transfer for all total fees (15uregen) from the buyer to the fee pool

transfer for seller payment (95uregen) from the buyer to the seller

This makes sense and the math adds up. But I wonder if it could lead to some confusion because from the resulting tx it looks like the buyer is paying all of the fees. I'm not sure how important it is too keep this detailed of a record on the TX or if it is much less efficient, but it seems like it could be useful for helpting to calculate total fees spent by buyer or seller (maybe for tax purposes?)

An example:

transfer for only buyer fees (10uregen) from the buyer to the fee pool

transfer for seller payment subtotal cost (100uregen) from the buyer to the seller

transfer for only seller fees (5uregen) from the seller to the fee pool

Personally, I would stick to the side of efficiency here and not introduce unnecessary state changes because of downstream accounting. If accounting is the consideration, I would rather deal with that by adding attributes to events, maybe enhancing EventBuyDirect?

Yeah, it might make sense to add that in EventBuyDirect then. I'm just assuming this would be useful for accounting, maybe we can validate that from a business use-case.

I agree, to keep track of accounting the event makes more sense

@paul121 @JeancarloBarrios added seller and buyer fees paid to EventBuyDirect, see b1a7ad2. Does that address this sufficiently?

That looks great

paul121

Ryan and I reviewed this PR today during the regen ledger office hours he is hosting and came out with an important question regarding this feature: Why is the seller paying the fee at time of purchase?

An alternative could be paying the seller fee when creating the sell order. There are few reasons this could be desirable:

There will be a scenario when the seller fee % changes after sell orders have been created. Paying the seller fee at the time of purchase means the seller can never guarantee what fee they might be charged in the future. All sell orders could automatically be closed when changing the seller fee % but this would create a bad UX.
Paying the seller fee when creating a sell order means the seller would be explicitly consenting to the current seller fee when signing their transaction.
It would be easier to track the exact seller fee amount (see my above comment)
This would help to prevent spamming the network of unreasonable sell orders. If a seller cancels the sell order, the fee has already been paid/burned, and benefits the network.

aaronc · 2024-02-15T19:43:04Z

Ryan and I reviewed this PR today during the regen ledger office hours he is hosting and came out with an important question regarding this feature: Why is the seller paying the fee at time of purchase?

An alternative could be paying the seller fee when creating the sell order. There are few reasons this could be desirable:

There will be a scenario when the seller fee % changes after sell orders have been created. Paying the seller fee at the time of purchase means the seller can never guarantee what fee they might be charged in the future. All sell orders could automatically be closed when changing the seller fee % but this would create a bad UX.

Paying the seller fee when creating a sell order means the seller would be explicitly consenting to the current seller fee when signing their transaction.

It would be easier to track the exact seller fee amount (see my above comment)

This would help to prevent spamming the network of unreasonable sell orders. If a seller cancels the sell order, the fee has already been paid/burned, and benefits the network.

I have to say that I strongly disagree with this. Sellers can change their pricing or close orders at any point in time. Making it so that they pay upfront would mean that the fees need to be escrowed. Also, they are paying for the fees in the sell denom which means they need to gather that capital in advance without having already gotten payment. If fees are low this might not be a problem, but if fees are non-trivial this capital requirement could be a barrier to entry. If we want an anti-spam fee, that should be something different IMHO.

paul121 · 2024-02-15T21:55:19Z

Yes, agree that needing the capital in advance could be a barrier to entry, and there could be other solutions for anti-spam fee. But we wondered if these things might be beneficial for the network. It would be a more significant change though.

The fact that fees could change after the sell order is created was our main concern. Perhaps it isn't a huge issue, but just wanted to make sure to flag that. It should at least be clear to the seller that the sell order may incur a fee when it is later closed.

aaronc · 2024-02-15T22:30:59Z

The fact that fees could change after the sell order is created was our main concern. Perhaps it isn't a huge issue, but just wanted to make sure to flag that. It should at least be clear to the seller that the sell order may incur a fee when it is later closed.

There will always be plenty of advanced notice around governance changes if sellers are concerned about fee changes.

JeancarloBarrios

I think it looks good I dont have any major comment LGTM

JeancarloBarrios · 2024-02-27T23:31:18Z

Yes, agree that needing the capital in advance could be a barrier to entry, and there could be other solutions for anti-spam fee. But we wondered if these things might be beneficial for the network. It would be a more significant change though.

The fact that fees could change after the sell order is created was our main concern. Perhaps it isn't a huge issue, but just wanted to make sure to flag that. It should at least be clear to the seller that the sell order may incur a fee when it is later closed.

"I like to compare it to the fees you see on the Apple App Store or similar platforms. Essentially, the ecosystem determines what fees are reasonable; under that mindset, what @aaronc suggests makes sense to me. However, I understand the concern of sellers having to adjust prices based on the fee. For quick implementation, it's acceptable because, as @aaronc mentioned, it requires a governance proposal, so it's unlikely to change significantly anytime soon. Later on, we can introduce a custom fee module that allows for more nuanced adjustments.

…onc/marketplace-fees-impl

aaronc · 2024-02-29T21:53:22Z

@paul121 @JeancarloBarrios I believe I have addressed all concerns. I have also added a new message MsgGovSendFromFeePool so that governance can actually manage the fee pool. This was initially missing. Please take another look when you get a chance. Once this has two approvals and if there are no objections I think I will go ahead and merge this so we can get closer to a deployment.

glandua · 2024-03-05T20:38:12Z

glandua · 2024-03-05T20:41:40Z

@aaronc you'd asked me to review this, mentioning that non technical folks like myself could engage in this productively. So far the only productive comment I have made is finding a broken link to cosmos module docs.

After reviewing the changes and discussions about adding a fee pool and governance / management of fees, it sounds like the team has made some solid decisions. While I'm not the tech expert to dive into the nitty-gritty, it seems like the next step would be for those who have the know-how to double-check the work. They'd ensure everything is up to snuff security-wise, works smoothly, and fits what the project aims to do. It's like making sure the foundation is strong before moving on!

aaronc · 2024-03-06T19:13:23Z

Thanks @glandua. I think the key high-level points to keep in mind here is that:

buyers could be charged a percentage fee on marketplace transactions on top of what they're paying for credits in the denon they're buying with
sellers could be charged a percentage fee off of what they're making from marketplace sales in the denon they're buying with
governance gets to decide those fees
fees are only deducted from the buyer and seller when the sale is finalized
REGEN gets burned and other coins get put in a fee pool
governance has to decide what to do with the fee pool of non-REGEN tokens

paul121 · 2024-03-07T18:28:18Z

I have also added a new message MsgGovSendFromFeePool so that governance can actually manage the fee pool. This was initially missing. Please take another look when you get a chance.

I reviewed these changes and they make sense to me. My only question is if we have a way for the marketplace account to grant access (likely authz authorization) for another account to manage these accrued fees more efficiently than via governance proposals. Can that authorization itself be created via a separate governance proposal? I assume this is already possible or could be added in a followup PR but just want to make sure there is a path for this.

aaronc · 2024-03-07T19:01:16Z

I have also added a new message MsgGovSendFromFeePool so that governance can actually manage the fee pool. This was initially missing. Please take another look when you get a chance.

I reviewed these changes and they make sense to me. My only question is if we have a way for the marketplace account to grant access (likely authz authorization) for another account to manage these accrued fees more efficiently than via governance proposals. Can that authorization itself be created via a separate governance proposal? I assume this is already possible or could be added in a followup PR but just want to make sure there is a path for this.

Authz should make this possible with no additional ledger dev needed.

JeancarloBarrios · 2024-03-12T23:32:10Z

governance has to decide what to do with the fee pool of non-REGEN tokens

I believe we can do that already. with authz itself

JeancarloBarrios · 2024-03-12T23:32:49Z

I reviewed it again and it LGTM!!!

aaronc · 2024-03-14T18:42:43Z

Thanks for the reviews @JeancarloBarrios and @paul121. Going to go ahead and merge this now.

aaronc added 11 commits January 30, 2024 12:00

feat(x/ecocredit/marketplace): add marketplace fee API

fa1208b

docs

3624c5d

cleanup

8e09e86

proto-gen

d0bfd3e

Merge branch 'main' of github.com:regen-network/regen-ledger into aar…

00563af

…onc/marketplace-fees

Merge branch 'main' of github.com:regen-network/regen-ledger into aar…

c589b42

…onc/marketplace-fees-impl

feat(x/ecocredit/marketplace): implement buyer and seller fees

6459263

validate basic

d22fe85

add go-mod-tidy-all

f82c581

WIP

0835631

make existing tests pass

c5b3f41

aaronc added 8 commits January 31, 2024 15:55

failing test

32d7bf1

tests

64b093c

Merge branch 'main' of github.com:regen-network/regen-ledger into aar…

494163f

…onc/marketplace-fees-impl

fix tests

aa7084d

fee params tests

9e27574

set fee params validation tests

77cfe06

add failing max_fee_amount tests

41b76f5

tests passing

dae82fa

aaronc commented Feb 5, 2024

View reviewed changes

tests

3ee7ed5

aaronc marked this pull request as ready for review February 5, 2024 19:55

lint

c2a362e

JeancarloBarrios self-requested a review February 6, 2024 16:01

refactor max fee amount to coin

478f3e4

aaronc commented Feb 12, 2024

View reviewed changes

proto/regen/ecocredit/marketplace/v1/tx.proto Show resolved Hide resolved

paul121 reviewed Feb 14, 2024

View reviewed changes

paul121 reviewed Feb 15, 2024

View reviewed changes

JeancarloBarrios approved these changes Feb 27, 2024

View reviewed changes

aaronc added 8 commits February 28, 2024 15:18

Merge branch 'main' of github.com:regen-network/regen-ledger into aar…

5ade325

…onc/marketplace-fees-impl

proto gen

c4a1c81

add seller and buyer fee data to EventBuyDirect

b1a7ad2

make max fee amount required

7af0973

add marketplace fee pool account

3376e76

fix tests

bd95846

add MsgGovSendFromFeePool

caa606c

fix coins

7442fe2

proto lint

dbf4e14

JeancarloBarrios requested review from paul121 and clevinson March 12, 2024 23:32

paul121 approved these changes Mar 13, 2024

View reviewed changes

aaronc merged commit 5fb7e60 into main Mar 14, 2024
34 of 37 checks passed

aaronc deleted the aaronc/marketplace-fees-impl branch March 14, 2024 18:42

feat(x/ecocredit/marketplace): implement buyer and seller fees #2154

feat(x/ecocredit/marketplace): implement buyer and seller fees #2154

Conversation

aaronc commented Jan 31, 2024 • edited by glandua Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Jan 31, 2024 • edited Loading

Codecov Report

aaronc Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeancarloBarrios Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul121 left a comment

Choose a reason for hiding this comment

aaronc commented Feb 15, 2024

paul121 commented Feb 15, 2024

aaronc commented Feb 15, 2024

JeancarloBarrios left a comment

Choose a reason for hiding this comment

JeancarloBarrios commented Feb 27, 2024

aaronc commented Feb 29, 2024

glandua commented Mar 5, 2024

Description

Author Checklist

Reviewers Checklist

glandua commented Mar 5, 2024 • edited Loading

aaronc commented Mar 6, 2024

paul121 commented Mar 7, 2024

aaronc commented Mar 7, 2024

JeancarloBarrios commented Mar 12, 2024

JeancarloBarrios commented Mar 12, 2024

aaronc commented Mar 14, 2024

aaronc commented Jan 31, 2024 •

edited by glandua

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

aaronc Feb 5, 2024 •

edited

Loading

JeancarloBarrios Feb 27, 2024 •

edited

Loading

glandua commented Mar 5, 2024 •

edited

Loading