feat(rollapp): allow rollapps with no native token #1654

mtsitrin · 2024-12-15T10:38:01Z

Description

Closes #XXX

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.

codecov · 2024-12-15T10:58:19Z

Codecov Report

Attention: Patch coverage is 78.37838% with 16 lines in your changes missing coverage. Please review.

Project coverage is 22.62%. Comparing base (02cbaec) to head (1e42c03).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
x/rollapp/genesisbridge/ibc_module.go	53.84%	4 Missing and 2 partials ⚠️
x/rollapp/types/genesis_bridge_data.go	66.66%	4 Missing and 2 partials ⚠️
x/rollapp/types/genesis_info.go	89.74%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1654      +/-   ##
==========================================
+ Coverage   22.59%   22.62%   +0.02%     
==========================================
  Files         590      590              
  Lines      129299   129316      +17     
==========================================
+ Hits        29217    29254      +37     
+ Misses      96191    96176      -15     
+ Partials     3891     3886       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zale144 · 2024-12-16T02:40:14Z

x/rollapp/genesisbridge/ibc_module.go

-		return uevent.NewErrorAcknowledgement(ctx, errorsmod.Wrap(err, "genesis bridge data: to IBC denom"))
+	// handle rollapp's denom metadata
+	var raDenomOnHUb string
+	if genesisBridgeData.GenesisInfo.NativeDenom.IsSet() {


could you comment this a bit more in detail

zale144 · 2024-12-16T02:43:20Z

x/rollapp/genesisbridge/ibc_module.go

 	for _, data := range genesisPackets {
 		if err := w.transferKeeper.OnRecvPacket(ctx, packet, data); err != nil {
 			return uevent.NewErrorAcknowledgement(ctx, errorsmod.Wrap(err, "handle genesis transfer"))
 		}
 	}

-	err = w.EnableTransfers(ctx, packet, ra, denom.Base)
+	err = w.EnableTransfers(ctx, packet, ra, raDenomOnHUb)


if genesisBridgeData.GenesisInfo.NativeDenom.IsSet() == false, raDenomOnHUb would be empty string, right?

right. as the rollapp doesn't have native denom

just making sure that w.EnableTransfers is ok with empty string, and also the event below

x/rollapp/types/genesis_bridge_data.go

zale144 · 2024-12-16T02:48:22Z

x/rollapp/types/genesis_info.go

@@ -33,14 +33,32 @@ func (gi GenesisInfo) GenesisTransferAmount() math.Int {
 	return total
 }

+// native denom is optional
 func (gi GenesisInfo) AllSet() bool {


AllSet might not be an appropriate name for this method anymore, since it doesn't check NativeDenom anymore

changed to Launchable

zale144 · 2024-12-16T02:54:39Z

x/rollapp/types/genesis_info.go

-		if _, exists := accountSet[a.Address]; exists {
-			return fmt.Errorf("duplicate genesis account: %s", a.Address)
+
+		if gi.InitialSupply.IsNil() {


what if InitialSupply is zero here?

will fail in if total.GT(gi.InitialSupply) {

could you maybe leave a comment about it, just to remove doubt

zale144 · 2024-12-16T02:57:36Z

x/rollapp/types/genesis_info.go

+	if a.Amount.IsNil() {
+		return fmt.Errorf("invalid amount: %s", a.Address)
+	}
+
+	if !a.Amount.IsPositive() {


these two checks can be merged into one

zale144 · 2024-12-16T03:08:17Z

x/rollapp/types/genesis_info.go

+		if !gi.InitialSupply.IsNil() && !gi.InitialSupply.IsZero() {
+			return ErrNoNativeTokenRollapp


seems that the error here is not really the lack of native denom, but the fact that initial supply is non-zero while having no native tokens.

zale144 · 2024-12-16T03:08:59Z

x/rollapp/types/genesis_info.go

+		if l := len(gi.Accounts()); l > 0 {
+			return ErrNoNativeTokenRollapp


same as above, for accounts

omritoptix

great!

omritoptix · 2024-12-15T13:57:18Z

x/rollapp/types/genesis_info.go

+	} else {
+		// if native denom is not set, initial supply must be 0 and no accounts
+		if !gi.InitialSupply.IsNil() && !gi.InitialSupply.IsZero() {
+			return ErrNoNativeTokenRollapp


Suggested change

return ErrNoNativeTokenRollapp

return ErrInvalidInitialSupply

omritoptix · 2024-12-15T13:57:49Z

x/rollapp/types/genesis_info.go

-		if !gi.InitialSupply.IsPositive() {
-			return ErrInvalidInitialSupply
+		if l := len(gi.Accounts()); l > 0 {
+			return ErrNoNativeTokenRollapp


Suggested change

return ErrNoNativeTokenRollapp

return ErrTooManyGenesisAccounts

x/rollapp/types/genesis_info.go

danwt

Should document on genesis info and genesis bridge info protos that native denom can be zero type, and why
should doc no IRO plan possible for such rollapps

danwt · 2024-12-16T09:54:37Z

x/rollapp/types/message_update_rollapp.go

 		return errorsmod.Wrapf(
 			errors.Join(gerrc.ErrInvalidArgument, err),
 			"invalid genesis info",
 		)
 	}

-	if !m.NewGenesisInfo.AllSet() {
+	if !m.NewGenesisInfo.Launchable() {


launchable is somewhat misleading, since rollapp is not launchable until after. pre launch time, which is not included
needs a better docstring

danwt · 2024-12-16T10:09:47Z

x/rollapp/keeper/rollapp.go

 	if update.GenesisInfo != nil {
-		if update.GenesisInfo.GenesisChecksum != "" {
-			current.GenesisInfo.GenesisChecksum = update.GenesisInfo.GenesisChecksum
-		}
-
-		if update.GenesisInfo.Bech32Prefix != "" {
-			current.GenesisInfo.Bech32Prefix = update.GenesisInfo.Bech32Prefix
-		}
-
-		if update.GenesisInfo.NativeDenom.Base != "" {
-			current.GenesisInfo.NativeDenom = update.GenesisInfo.NativeDenom
-		}
-
-		if !update.GenesisInfo.InitialSupply.IsNil() {
-			current.GenesisInfo.InitialSupply = update.GenesisInfo.InitialSupply
-		}
-
-		// Frontend always passes new value
-		current.GenesisInfo.GenesisAccounts = update.GenesisInfo.GenesisAccounts
+		current.GenesisInfo = *update.GenesisInfo
 	}


this definitely fine?

danwt · 2024-12-16T10:13:02Z

x/rollapp/genesisbridge/ibc_module.go

+// It also calls the after transfers enabled hook (used to settle IRO plans)
 func (w IBCModule) EnableTransfers(ctx sdk.Context, packet channeltypes.Packet, ra *types.Rollapp, rollappIBCtrace string) error {


should say it can be empty

danwt · 2024-12-16T10:16:03Z

x/rollapp/types/genesis_info.go

+		if l := len(gi.Accounts()); l > 0 {
+			return errorsmod.Wrap(ErrNoNativeTokenRollapp, "non empty genesis accounts")
 		}


why define l

danwt · 2024-12-16T10:18:26Z

x/rollapp/types/genesis_info.go

+	// if native denom is not set, initial supply must be 0 and no accounts
+	if !gi.NativeDenom.IsSet() {
+		if !gi.InitialSupply.IsNil() && !gi.InitialSupply.IsZero() {
+			return errorsmod.Wrap(ErrNoNativeTokenRollapp, "non zero initial supply")
 		}


this doesn't check initial supply is zero, since it can be nil

danwt · 2024-12-16T10:19:32Z

x/rollapp/types/genesis_info.go

+		if total.GT(gi.InitialSupply) {
+			return ErrInvalidInitialSupply
 		}


supply != genesis bridge transfers (e.g can have 1B tokens with 1M transferred on genesis)

danwt · 2024-12-16T10:20:43Z

x/rollapp/types/message_update_rollapp.go

+	// genesis info is not validated here, as we allow to update partial fields
+	// it will be validated when trying to apply the update


fine grained update is removed now though

danwt

approved,
check #1654 (comment)

(cherry picked from commit a9d5d6b)

github-actions bot added the dym-internal label Dec 15, 2024

mtsitrin added 4 commits December 15, 2024 12:40

allowing no native token for rollapp

ed01a27

fixed validateBasic

695130a

UT

e8d7160

fixed ibc UT

1e42c03

mtsitrin force-pushed the feat/allow_rollapp_with_no_native_token branch from 3dd0742 to 1e42c03 Compare December 15, 2024 10:40

linter

af88ea3

mtsitrin marked this pull request as ready for review December 15, 2024 13:33

mtsitrin requested a review from a team as a code owner December 15, 2024 13:33

updating genesis info as a whole

dfd9c37

zale144 requested changes Dec 16, 2024

View reviewed changes

omritoptix previously approved these changes Dec 16, 2024

View reviewed changes

renames and cleanups

9ff598e

mtsitrin dismissed omritoptix’s stale review via 9ff598e December 16, 2024 08:53

mtsitrin requested review from zale144 and omritoptix December 16, 2024 09:01

danwt requested changes Dec 16, 2024

View reviewed changes

danwt reviewed Dec 16, 2024

View reviewed changes

pr comments

eff8a62

omritoptix approved these changes Dec 16, 2024

View reviewed changes

danwt approved these changes Dec 16, 2024

View reviewed changes

omritoptix merged commit a9d5d6b into main Dec 16, 2024
5 checks passed

omritoptix deleted the feat/allow_rollapp_with_no_native_token branch December 16, 2024 11:42

omritoptix pushed a commit that referenced this pull request Dec 16, 2024

feat(rollapp): allow rollapps with no native token (#1654)

a3e19dd

(cherry picked from commit a9d5d6b)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rollapp): allow rollapps with no native token #1654

feat(rollapp): allow rollapps with no native token #1654

mtsitrin commented Dec 15, 2024

codecov bot commented Dec 15, 2024

zale144 Dec 16, 2024

zale144 Dec 16, 2024

mtsitrin Dec 16, 2024

zale144 Dec 16, 2024

zale144 Dec 16, 2024

mtsitrin Dec 16, 2024

zale144 Dec 16, 2024

mtsitrin Dec 16, 2024

zale144 Dec 16, 2024

zale144 Dec 16, 2024

zale144 Dec 16, 2024

zale144 Dec 16, 2024

omritoptix left a comment

omritoptix Dec 15, 2024

omritoptix Dec 15, 2024

danwt left a comment

danwt Dec 16, 2024

danwt Dec 16, 2024

danwt Dec 16, 2024

danwt Dec 16, 2024

danwt Dec 16, 2024

danwt Dec 16, 2024

mtsitrin Dec 16, 2024

danwt Dec 16, 2024

danwt left a comment

		if !gi.InitialSupply.IsNil() && !gi.InitialSupply.IsZero() {
		return ErrNoNativeTokenRollapp

		if l := len(gi.Accounts()); l > 0 {
		return ErrNoNativeTokenRollapp

	return ErrNoNativeTokenRollapp
	return ErrInvalidInitialSupply

	return ErrNoNativeTokenRollapp
	return ErrTooManyGenesisAccounts

		// It also calls the after transfers enabled hook (used to settle IRO plans)
		func (w IBCModule) EnableTransfers(ctx sdk.Context, packet channeltypes.Packet, ra *types.Rollapp, rollappIBCtrace string) error {

		// genesis info is not validated here, as we allow to update partial fields
		// it will be validated when trying to apply the update

feat(rollapp): allow rollapps with no native token #1654

feat(rollapp): allow rollapps with no native token #1654

Conversation

mtsitrin commented Dec 15, 2024

Description

codecov bot commented Dec 15, 2024

Codecov Report

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

danwt 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

danwt left a comment

Choose a reason for hiding this comment