feat(IRO): IRO module implementation #1201

mtsitrin · 2024-09-05T19:20:47Z

Description

Implement IRO based on this ADR: https://www.notion.so/dymension/ADR-x-IRO-533f1c2f5a2345838150dd8e954c99c6?pvs=4

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.

PR review checkboxes:

I have...

SDK Checklist

Full security checklist here

For Reviewer:

Confirmed the correct type prefix in the PR title
Reviewers assigned
Confirmed all author checklist items have been addressed

After reviewer approval:

In case the PR targets the main branch, PR should not be squash merge in order to keep meaningful git history.
In case the PR targets a release branch, PR must be rebased.

omritoptix

There is a lack of consistency in variables naming in the code when it comes to describing costs as int/dec or with denom (i.e sdk.Coin)

for example:

costCoin := sdk.NewCoin(appparams.BaseDenom, cost)
totalCost, takerFeeCoin := k.AddTakerFee(costCoin, k.GetParams(ctx).TakerFee)

costCoin - refers to amout of coins
totalCost - refers to amount of coins
takerFeeCoin - refers to amount of coins

For me confusing and inconsistent.

I suggest:
costAmt - the int/dec repr (so just a number)
cost - the actual cost of amount + denom .

so in the following example:

costCoin -> cost
totalCost -> costWithTakerFee
takerFeeCoin -> takerFee

x/rollapp/keeper/rollapp.go

omritoptix · 2024-09-09T12:56:07Z

x/rollapp/keeper/rollapp.go

 	}

+	rollapp.GenesisInfo.Sealed = true


this function is called when bonding a sequencer.
assuming no IRO, seems like we don't validate all genesis fields exists before sealing.

validated few lines above
if !rollapp.AllImmutableFieldsAreSet()

omritoptix · 2024-09-09T12:57:29Z

x/sequencer/keeper/msg_server_create_sequencer.go

+
+		// check pre launch time
+		if rollapp.PreLaunchTime.After(ctx.BlockTime()) {
+			return nil, types.ErrBeforePreLaunchTime


assuming no IRO, currently this field is not set (i.e the preLaunchTime).
how does the non-iro flow looks like in that case? and why do we validate it in the non-iro flow?

if it's not set, the expression is always false

I see. imo this implicitness is confusing. we can add a comment to clarify or alternatively explictly check this in the case this sequencer is launched for rollapp with IRO.

also thought of suggesting it would be cleaner to move preLaunchTime to only live on the IRO object if it's only IRO related but than checked the IRO object and I see it also has a preLaunchTime. and seems like it's a duplicate var living in both places. so I think worth just leaving it on the IRO, change it's name to EndTime and make sure a rollapp can't start until rollapp.iro.EndTime is over.

would also address #1201 (comment)

actually I think it's better to have preLaunchTime as rollapp's field, as it might be enforced for different reasons in the future, not only from IRO.
I left it on the IRO plan object as well for clearity, but it's not used and can be removed from the plan store if u prefer.

actually I think it's better to have preLaunchTime as rollapp's field, as it might be enforced for different reasons in the future, not only from IRO.

I'm fine with that.

I left it on the IRO plan object as well for clearity, but it's not used and can be removed from the plan store if u prefer.

also fine with that, but imo we need to change it's name to EndTime cause I don't see the logic of Pre-Launch time on the IRO object, especially with it's comment related to rollapp.

omritoptix · 2024-09-09T13:00:07Z

x/rollapp/keeper/rollapp.go

+// UpdateRollappWithIROPlan seals the rollapp genesis info and sets it pre launch time according to the iro plan end time
+// - GenesisInfo fields must be set
+// - Rollapp must not be Launched
+func (k Keeper) SealGenesisInfoWithLaunchTime(ctx sdk.Context, rollapp *types.Rollapp, preLaunchTime time.Time) error {


why do we couple the SealGenesis with setting launch time?
If I want to seal genesis for rollapp without an IRO plan (and hence without LaunchTime) ?
currently the sealing logic is maintained in multiple places. here and on SetRollappAsLaunched. We should dry it out.

genesis info sealing logic is rollapp.GenesisInfo.Sealed = true
not sure what there's to dry out
SetRollappAsLaunched and SealGenesisInfoWithLaunchTime have different pre-conditions.

genesis info sealing logic is rollapp.GenesisInfo.Sealed = true

not really, cause you always want to validate that all pre-requirements are set.
currently the pre-requirements check are delegated to the caller vs just creating a Seal function which validates pre-requirements in one place and seals.

so now you're checking both independetly and sealing in:

SetIROPlanToRollapp

SetRollappAsLaunched

the next caller will also need to implement the checks and the sealing vs just calling Seal

x/rollapp/types/rollapp.go

x/iro/keeper/trade.go

x/iro/types/iro.pb.go

omritoptix · 2024-09-09T23:07:29Z

x/rollapp/transfergenesis/ibc_module.go

+	if rollapp.GenesisState.TransfersEnabled {
+		return false
+	}
+	if rollapp.PreLaunchTime.IsZero() {


wby do we check it implicity vs explicity, i.e a rollapp iwth an unsettled IRO plan which has finished?

it's the same. if IRO exists, preluanch is not zero. if it;s settled, than TransfersEnabled .
added comment to clarify

yea I understand it's the same, just don't understand whats the reasoning for checking this vs GetPlanByRollap result. I assume there is a reason.

x/rollapp/transfergenesis/ibc_module.go

… default

omritoptix

Some var name suggestions and also a duplicate variable it seems but nothing which is a blocker.

you can either address now or merge and open a technical debt issue with link to the comments and address them later.

x/iro/keeper/trade.go

x/iro/types/params.go

omritoptix · 2024-09-20T15:13:33Z

x/rollapp/transfergenesis/ibc_module.go

+	if rollapp.GenesisState.TransfersEnabled {
+		return false
+	}
+	if rollapp.PreLaunchTime.IsZero() {


yea I understand it's the same, just don't understand whats the reasoning for checking this vs GetPlanByRollap result. I assume there is a reason.

omritoptix · 2024-09-20T15:41:42Z

x/sequencer/keeper/msg_server_create_sequencer.go

+
+		// check pre launch time
+		if rollapp.PreLaunchTime.After(ctx.BlockTime()) {
+			return nil, types.ErrBeforePreLaunchTime


I see. imo this implicitness is confusing. we can add a comment to clarify or alternatively explictly check this in the case this sequencer is launched for rollapp with IRO.

also thought of suggesting it would be cleaner to move preLaunchTime to only live on the IRO object if it's only IRO related but than checked the IRO object and I see it also has a preLaunchTime. and seems like it's a duplicate var living in both places. so I think worth just leaving it on the IRO, change it's name to EndTime and make sure a rollapp can't start until rollapp.iro.EndTime is over.

would also address #1201 (comment)

VictorTrustyDev · 2024-10-03T16:27:16Z

proto/dymensionxyz/dymension/iro/genesis.proto

+message GenesisState {
+  // Params defines params for x/sponsorship module.
+  Params params = 1 [ (gogoproto.nullable) = false ];
+  // VoterInfos hold information about voters.


VictorTrustyDev · 2024-10-03T16:41:47Z

x/iro/keeper/claim.go

+	k.SetPlan(ctx, plan)
+
+	// Emit event
+	err = ctx.EventManager().EmitTypedEvent(&types.EventClaim{


@zale144 I guess this place should use the sdk util method you wrote, instead of directly like this?

VictorTrustyDev · 2024-10-03T17:30:07Z

x/iro/types/types.go

@@ -0,0 +1 @@
+package types


mtsitrin added 30 commits August 26, 2024 15:52

genereated proto files

e48ae3f

fixed proto

b6ecec8

scaffold

ec02a4d

store keys

c5ee033

initial store getters

70f179e

implemented create plan

ffbb44a

implemented trade methods and linear bonding curve

4f45328

implemented the cur

59c6fed

wired keeper, and started testing

48eb399

implemented bonding curve and create plan

ac7ff90

first UT pass

030f450

bond curve fix and UT

abfb8c5

trade test

0ba4c5c

added curve params to the plan

b9ebf1c

using curve from plan

d9a1818

Merge branch 'main' into feature/IRO

a6bf24f

refactor genesistransfer. added transferEnabled hook

bca38d8

UT compiles

33c09d6

genesis transfer UT

8b69ae8

genesis test

0d18586

implemented queries

eb7fcf1

module account for plan

1033813

fixed module account

2e1d0c3

fixed validation test

3e74625

liquidity bootstrap wip

3178c9f

fixed claim. added buying allocation limit

f55fada

creating pool

d1c22d6

UT wip

63e25d1

IRO creation fee

63adb6d

linter

35b464e

Merge branch 'main' into feature/IRO

155d095

mtsitrin marked this pull request as ready for review September 9, 2024 11:31

mtsitrin requested a review from a team as a code owner September 9, 2024 11:31

mtsitrin requested a review from omritoptix September 9, 2024 11:31

omritoptix requested changes Sep 9, 2024

View reviewed changes

omritoptix reviewed Sep 9, 2024

View reviewed changes

mtsitrin added 5 commits September 10, 2024 12:21

dry out some code

8c9c7ce

fixed sequencer creation bug. added UT

2b24e32

split params for minimal incentives values

4a548cb

transfer genesis validated only when IRO exists. transfers enabled by…

c1c9e70

… default

renames and cleanup

61accb0

mtsitrin requested a review from omritoptix September 10, 2024 12:21

mtsitrin added 2 commits September 17, 2024 12:45

Merge branch 'main' into feature/IRO

3a09f0d

goimports

9a259a1

github-actions bot added the dym-internal label Sep 17, 2024

omritoptix previously approved these changes Sep 20, 2024

View reviewed changes

pr comments

73da686

mtsitrin dismissed omritoptix’s stale review via 73da686 September 23, 2024 06:56

mtsitrin added 2 commits September 23, 2024 09:58

Merge branch 'main' into feature/IRO

5c0fc8d

Merge branch 'main' into feature/IRO

3fb6b06

anhductn2001 mentioned this pull request Sep 24, 2024

Update e2e-tests for IRO module dymensionxyz/e2e-tests#438

Closed

mtsitrin added 5 commits September 24, 2024 13:04

fixed cli

11789f3

This reverts commit 11789f3.

b0cb7b0

added cli UT

f6a96ee

linter

2113bf6

actually fixed

94a4add

mtsitrin merged commit 5601240 into main Sep 24, 2024
116 of 128 checks passed

mtsitrin deleted the feature/IRO branch September 24, 2024 13:45

VictorTrustyDev reviewed Oct 3, 2024

View reviewed changes

omritoptix mentioned this pull request Oct 18, 2024

feat(IRO): allow iro buy with dym amount as input #1318

Merged

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(IRO): IRO module implementation #1201

feat(IRO): IRO module implementation #1201

mtsitrin commented Sep 5, 2024

omritoptix left a comment

omritoptix Sep 9, 2024

mtsitrin Sep 10, 2024

omritoptix Sep 9, 2024

mtsitrin Sep 10, 2024

omritoptix Sep 20, 2024

mtsitrin Sep 23, 2024

omritoptix Sep 23, 2024

omritoptix Sep 9, 2024

mtsitrin Sep 10, 2024

mtsitrin Sep 10, 2024

omritoptix Sep 21, 2024

omritoptix Sep 9, 2024

mtsitrin Sep 10, 2024

mtsitrin Sep 10, 2024

omritoptix Sep 20, 2024

omritoptix left a comment

omritoptix Sep 20, 2024

omritoptix Sep 20, 2024

VictorTrustyDev Oct 3, 2024

VictorTrustyDev Oct 3, 2024

VictorTrustyDev Oct 3, 2024

feat(IRO): IRO module implementation #1201

feat(IRO): IRO module implementation #1201

Conversation

mtsitrin commented Sep 5, 2024

Description

omritoptix left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omritoptix left a 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