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

Ingawei/man 1823 shift market tiers down a level #2933

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

Conversation

ingawei
Copy link
Collaborator

@ingawei ingawei commented Oct 8, 2024

Run by doing
SELECT * FROM update_contract_tiers_efficient(50, '1970-01-01 00:00:00'::timestamp);

Shifts tiers down a level and gets rid of the 'Basic' tier, for simplicity and so that Mana and Cash markets can have tiers on the same scale (100, 1k, 10k, 100k). SQL script backfills old markets so they are the correct downgraded tier.

Copy link

linear bot commented Oct 8, 2024

Copy link

vercel bot commented Oct 8, 2024

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

3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
dev ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 9:41pm
docs ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 9:41pm
prod ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 9:41pm

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.

Thanks for working on unifying the tiers! It'll be great to eliminate the 'basic' tier which is very confusing atm.

I will say I have reservations about jumping to this cached tier approach before we've tried splitting out the totalLiquidity into its own column (which is as easy as adding another line to the contract_populate_cols function) and computing the tiers on the fly (this could be a line in the contracts table, like how description_fts is calculated). If we computed the tiers on the fly, this PR would be a lot simpler SQL-wise and avoid code duplication between typescript and sql. It'd also let us experiment with different liquidity requirements by changing a few numbers in typescript rather than having to edit typescript, the sql function, and our 150k contracts every time we changed something.

WHERE c.id = tu.id
AND (c.tier IS DISTINCT FROM tu.new_tier OR c.tier IS NULL);

GET DIAGNOSTICS batch_updated = ROW_COUNT;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've never used this type of batch editing before, so not sure how to evaluate it. I don't see a working script where you call this function so I can't evaluate how you'll run it, but I'd make sure to test this on dev and only test it on one contract to begin with. Also, once the script works on dev with a certain batch size, you'll still want to use a small batch on prod because of the higher activity.

I've only used typescript to grab contracts, then update them using ts logic. Is there a reason you wrote the script in sql instead of how our other scripts work?

!('marketTier' in ct.data()) &&
'totalLiquidity' in ct.data()
)
return !('marketTier' in ct.data()) && 'totalLiquidity' in ct.data()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this file supposed to do? It uses firebase and won't change anything on our contracts

$$ language plpgsql;

-- Commit the transaction to save the functions
commit;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need to commit a function

ELSE RETURN 'play';
END IF;
END;
$$ language plpgsql immutable;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This logic looks good, but ofc be sure to test it on dev first.

Thinking about this a bit more, I don't love how we'll have this logic duplicated in sql and ts, though. If we edit the FIXED_ANTE in ts, now we have to edit fixed_ante in sql and then also update every contract. That makes experimentation very slow. Have we tried putting totalLiquidity into its own native column and then computing the tiers on the fly? That would be much easier than having to precompute all of this.

</Tooltip>
</span>
)}
{!!contract.isSpicePayout && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

The spice is no longer!

@ingawei
Copy link
Collaborator Author

ingawei commented Oct 10, 2024

Thanks for working on unifying the tiers! It'll be great to eliminate the 'basic' tier which is very confusing atm.

I will say I have reservations about jumping to this cached tier approach before we've tried splitting out the totalLiquidity into its own column (which is as easy as adding another line to the contract_populate_cols function) and computing the tiers on the fly (this could be a line in the contracts table, like how description_fts is calculated). If we computed the tiers on the fly, this PR would be a lot simpler SQL-wise and avoid code duplication between typescript and sql. It'd also let us experiment with different liquidity requirements by changing a few numbers in typescript rather than having to edit typescript, the sql function, and our 150k contracts every time we changed something.

Yeah this was a big back and forth that happened when I first implemented tiers. The issue is it's not a hard number that if it's above and below it's a certain tier. For multi choice, a lower tier with 100 answers would cost the same as a binary with a higher tier. So it's not a simple SQL query if you're for example searching for a bunch of markets within a certain tier.

@IanPhilips
Copy link
Collaborator

Ah, so in the multichoice case where you can add answers, we could store the required ante, right? So we could still avoid having the hard-coded strings

@ingawei
Copy link
Collaborator Author

ingawei commented Oct 10, 2024

Ah, so in the multichoice case where you can add answers, we could store the required ante, right? So we could still avoid having the hard-coded strings

I see so you want to store total liquidity and base ante? So then when you filter by tier you just look for contracts in which total liquidity = ante * 10^tier?

@IanPhilips
Copy link
Collaborator

IanPhilips commented Oct 10, 2024

Ah, so in the multichoice case where you can add answers, we could store the required ante, right? So we could still avoid having the hard-coded strings

I see so you want to store total liquidity and base ante? So then when you filter by tier you just look for contracts in which total liquidity = ante * 10^tier?

I think we would just save base_ante for indie mc markets, and it would work like sth if base_ante then base_ante > 10**ante_tier else total_liquidity>10**liquidity_tier, wouldn't you just need the ante if the liquidity didn't matter, so why the equals sign?

@IanPhilips
Copy link
Collaborator

Oh I forgot to ask, where is get_tier_from_liquidity_efficient called after backfilling? You'll have to call it when adding or removing liquidity, right?

@ingawei
Copy link
Collaborator Author

ingawei commented Oct 11, 2024

It's here in add-liquidity.ts:

   await updateContract(tx, contractId, {
      subsidyPool: FieldVal.increment(subsidyAmount),
      totalLiquidity: FieldVal.increment(subsidyAmount),
      marketTier: getTierFromLiquidity(
        contract,
        contract.totalLiquidity + subsidyAmount
      ),
    })

I made the get_tier_from_liquidity_efficient function just for the purpose of backfilling which I'm now realizing was pretty dumb lol. It seemed simpler at the time to do it in SQL than to do it with ts, mainly cause I couldn't find any example scripts backfilling to supabase

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.

2 participants