-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: Add referral feature #2355
Conversation
coordinator/src/db/user.rs
Outdated
users::version.eq(version), | ||
)) | ||
.execute(conn)?; | ||
let affected_rows = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic above would have done an update on conflict. Isn't it safer the way it was before?
bc56097
to
9daf509
Compare
Oh boy, you rebased it on top of |
coordinator/src/routes.rs
Outdated
@@ -167,10 +166,6 @@ pub fn router( | |||
// pass registration | |||
.route("/api/register", post(post_register)) | |||
.route("/api/users", post(post_register)) | |||
.route( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep this endpoint as it's useful for debugging. I've moved it into admin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A database query would also do, though.. :)
4e15aab
to
6673e84
Compare
TODO:
|
I didn't expect a lot of changes on the PR #2354, but you can simple rebase again on main.. There haven't been many conflicts. |
3d6cfc0
to
0f1ca45
Compare
8675cc1
to
b53e0c3
Compare
3d50c00
to
cdd499b
Compare
6337229
to
3619217
Compare
Boy, this is harder than I thought... In the last patch I've added that a referred user gets a bonus and rewrote the referring logic. We might want to squash those commits because I aint gonna rebase this 🙈. In the final testing I've introduced a bug though: a user who enters a wrong referral code should not get a referral bonus. I'll see to work on this on the weekend. But please review already. |
b53e0c3
to
e245c9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't review it in a lot of detail, but it looks good to me and if it works it works 🙂
Overall this PR adds far more features than I would have anticipated for the first referral feature. Maybe a smaller change upon which we could have iterated would have made the change easier. 🙈
@@ -0,0 +1,4 @@ | |||
-- Your SQL goes here | |||
-- TODO: calculate the matching fee |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary, since the trade params are only used temporarily. However, if you still want to do it I think its not hard to do.
Either way, lets get rid of that TODO :)
use time::OffsetDateTime; | ||
|
||
/// A user's referral bonus status may be active for this much days at max | ||
const MAX_DAYS_FOR_ACTIVE_REFERRAL_STATUS: i64 = 30; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Shouldn't that go to the settings?
coordinator/src/db/trades.rs
Outdated
trader_pubkey: PublicKey, | ||
) -> Result<Vec<crate::trade::models::Trade>> { | ||
// only take trades with open or closed positions | ||
let subquery = positions::table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 That should not be required anymore, since we only create a trade if the dlc protocol finished.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :)
@@ -128,6 +195,8 @@ pub fn login_user( | |||
fcm_token: token.clone(), | |||
version: version.clone(), | |||
last_login, | |||
// TODO: this breaks the used referral code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Why will this break the used referral code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Break might be the wrong term, but since we allow users to register on login their referral code might not be forwarded. Alternatively we would need to add the referral code on login as well but that's a bit weird.
So imho, this function here is wrong, we shouldn't create a new user on login. :)
coordinator/src/orderbook/trading.rs
Outdated
let fee_discount = status.referral_fee_bonus; | ||
let fee_percent = fee_percent - (fee_percent * fee_discount); | ||
|
||
tracing::debug!(%fee_discount, total_fee_percent = %fee_percent, "Fee discount calculated"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 We should add the trader_id
here as otherwise this log message gets pretty useless.
coordinator/src/referrals.rs
Outdated
let user = db::user::get_user(connection, &trader_pubkey)?.context("User not found")?; | ||
|
||
// we sort by tier_level, higher tier means better bonus | ||
bonus_status.sort_by(|a, b| b.tier_level.cmp(&a.tier_level)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 will that work? below are the bonus tiers. Any referred user will get the Referent
tier as bonus tier when installing the app. Now this user might refer 50 user, thus he should get a 50% fee_rabate
, but that won't happen because 4>3 so he is stuck on the Referent
bonus tier until it gets inactive.
(0, 0, 0.0, 'Referral', true),
(1, 3, 0.20, 'Referral', true),
(2, 5, 0.35, 'Referral', true),
(3, 10, 0.50, 'Referral', true),
(4, 0, 0.1, 'Referent', true);
Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, I don't think we should order by bonus tier, which i think is not needed at all, just ordering by the fee_rabate
should be sufficient, as we always want to give the user the best fee_rabate
he is eligible for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any referred user will get the Referent tier as bonus tier when installing the app.
Not quite, only if you have a valid referral code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by adding the fee_rebate into the bonus table.
coordinator/src/referrals.rs
Outdated
} | ||
|
||
// if he has no one referred so far, we check for an active referent bonus | ||
if let Some(status) = has_active_referent_bonus(connection, bonus_status.as_slice(), &user)? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Shouldn't that be already covered in the bonus status above
db::bonus_status::active_status_for_user(connection, &trader_pubkey)?;
doesn't differentiate between Referent
or Referral
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, removed
coordinator/src/scheduler.rs
Outdated
pool: Pool<ConnectionManager<PgConnection>>, | ||
) -> Result<Job, JobSchedulerError> { | ||
Job::new_async(schedule, move |_, _| { | ||
let mut conn = pool.get().expect("To be able to get a db connection"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 do not expect here #2371
mobile/lib/features/brag/brag.dart
Outdated
return const CircularProgressIndicator(); | ||
} | ||
return QrImageView( | ||
data: "https://referral.10101.finance?referral=${snapshot.data!.referralCode}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
mobile/native/src/api.rs
Outdated
/// The user has been referred and gets a bonus | ||
Referent, | ||
/// This bonus is part of a promotion | ||
Promotion, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 this doesn't exist anymore
nit: It would be great if we could show the |
No need for all this text. Imho this is self-explanatory.
In the app, we always register the user first, so we should do this here as well.
By default a referral status can be used for up to 1 month.
This patch also foresees a promo code. it has not been fully pulled through yet though.
These are the rules: - if a user gets referred, he gets a new `Referent` bonus. - after X referred users, the referring user gets a `Referral` bonus. - a `Referral` bonus is higher than a `Referent` bonus, hence, if a user got referred, but refers himself, he will only get the referral bonus. The bonus levels are as following for the time being: - get referred: -10% matching fee - refer 3 (active users - trade > 1): -20% - refer 5: -35% - refer 10: -50% The referral bonus is active for max 1 month.
This will make the text automatically scale down. Also, I've incorporated a feedback I've received a long time ago : Removing the opening price
I propose doing this in a follow-up PR |
I thought I'd create a draft PR so that you can already have a looks.
Open todos:
6673e84
(#2355)Left user was referred by right user. I've manipulated the referral bonus requirements so that 1 trade pushed the referrer to level 3.
On the trade screen you see that the fee has been deducted. I didn't do the math, but it should be ~60% less.
Screen.Recording.2024-04-04.at.5.12.31.pm.mov
resolves #2341