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

[aaronc]

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...

• [ ] included the correct type prefix in the PR title
• [ ] added ! to the type prefix if API or client breaking change
• [ ] targeted the correct branch (see PR Targeting)
• [ ] provided a link to the relevant issue or specification
• [ ] followed the guidelines for building modules
• [ ] included the necessary unit and integration tests
• [ ] added a changelog entry to CHANGELOG.md
• [ ] included comments for documenting Go code
• [ ] updated the relevant documentation or specification
• [ ] reviewed "Files changed" and left comments if necessary
• [ ] confirmed all CI checks have passed

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)

[codecov[bot]]

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%)	:arrow_up:
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%)	:arrow_up:
...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%)	:arrow_up:
x/ecocredit/marketplace/keeper/msg_buy_direct.go	79.31% <79.31%> (+1.18%)	:arrow_up:
...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%)	:arrow_up:
... and 1 more

[aaronc]

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]

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]

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]

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.

[aaronc]

@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]

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...

• [ ] included the correct type prefix in the PR title
• [ ] added ! to the type prefix if API or client breaking change
• [ ] targeted the correct branch (see PR Targeting)
• [ ] provided a link to the relevant issue or specification
• [ ] followed the guidelines for building modules
• [ ] included the necessary unit and integration tests
• [ ] added a changelog entry to CHANGELOG.md
• [ ] included comments for documenting Go code
• [ ] updated the relevant documentation or specification
• [ ] reviewed "Files changed" and left comments if necessary
• [ ] confirmed all CI checks have passed

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)

Note that the cosmos module docs have moved here: https://docs.cosmos.network/main/build/building-modules/intro

[glandua]

@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]

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]

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]

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]

• 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]

I reviewed it again and it LGTM!!!

[aaronc]

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