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

Trading ban #2853

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Trading ban #2853

wants to merge 17 commits into from

Conversation

SirSaltyy
Copy link
Collaborator

No description provided.

Copy link

vercel bot commented Sep 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 18, 2024 9:54pm
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 18, 2024 9:54pm
prod ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 18, 2024 9:54pm

@SirSaltyy
Copy link
Collaborator Author

@mantikoros if u think someone else should review let me know. Also, I might need help resolving the conflict as I think the conflict editor is being dumb (or I am) and I don't want to accidentally undo someone's changes.

@SirSaltyy
Copy link
Collaborator Author

Oh, I just did a trading ban (rather than sweepcash and mana-specific), because I couldn't think of a single scenario where we'd want to ban someone from one and not just both.

Admin accounts not being allowed to trade sweepcash is covered by other logic.

@mantikoros
Copy link
Collaborator

I can think of several cases where we'd only want to ban sweeps trading but not mana trading: Being an admin account is an overly restrictive condition. Future Manifold employees will not be admins. Non-employees who are connected to Manifold may also need to be banned. Users may also voluntarily request to be banned from trading as part of our responsible gaming program.

Copy link
Collaborator

@mantikoros mantikoros left a comment

Choose a reason for hiding this comment

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

Still think we should have separate bans for sweeps + mana, given the cases I described in a comment above. It's much easier to make this change now than when everything is in production and we have a more generic ban in use...

Copy link
Collaborator

@IanPhilips IanPhilips left a comment

Choose a reason for hiding this comment

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

Nice! Almost done

@@ -14,15 +14,15 @@ const bodySchema = z
})
.strict()

export const banuser = authEndpoint(async (req, auth) => {
export const banUserFromPosting = authEndpoint(async (req, auth) => {
const { userId, unban } = validate(bodySchema, req.body)
Copy link
Collaborator

Choose a reason for hiding this comment

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

best to name properties in the positive valence, like ban rather than unban

userId,
})
await updateUser(pg, userId, {
isBannedFromMana: !unban,
Copy link
Collaborator

Choose a reason for hiding this comment

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

would prefer isBannedFromManaTrading

userId,
})
await updateUser(pg, userId, {
isBannedFromSweepcash: !unban,
Copy link
Collaborator

Choose a reason for hiding this comment

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

isBannedFromSweeipcashTrading

@@ -16,11 +16,15 @@ import { Answer } from 'common/answer'
import { CpmmState, getCpmmProbability } from 'common/calculate-cpmm'
import { ValidatedAPIParams } from 'common/api/schema'
import { onCreateBets } from 'api/on-create-bet'
<<<<<<< trading-ban
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix these merge errors

@@ -307,11 +319,16 @@ export const fetchContractBetDataAndValidate = async (
'You must be kyc verified to trade on sweepstakes markets.'
)
}
<<<<<<< trading-ban
Copy link
Collaborator

Choose a reason for hiding this comment

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

more merge indicators to remove

@@ -307,11 +319,16 @@ export const fetchContractBetDataAndValidate = async (
'You must be kyc verified to trade on sweepstakes markets.'
)
}
<<<<<<< trading-ban
if (user.userDeleted) {
throw new APIError(403, 'You are banned or deleted.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

just deleted now

@@ -322,7 +339,12 @@ export const fetchContractBetDataAndValidate = async (
log(
`Loaded user ${user.username} with id ${user.id} betting on slug ${contract.slug} with contract id: ${contract.id}.`
)

if (user.isBannedFromMana && contract.token !== 'CASH') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

might as well check for the mana token

@@ -312,15 +312,29 @@ export const updateMarket = curriedAPI('market/:contractId/update')

export const updateUser = curriedAPI('me/update')

export function banUser(params: { userId: string; unban?: boolean }) {
return call(getApiUrl('ban-user'), 'POST', params)
export function banUserFromPosting(params: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't how we create api endpoints anymore, but I guess this is okay. In the future you should use the schema.ts

db,
'users',
...defaultFields,
'isBannedFromPosting',
Copy link
Collaborator

Choose a reason for hiding this comment

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

just add these to the default fields, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants