feat(rollapp): new rollapp registration flow #980
Conversation
zale144
changed the title
…ts, emit rollapp created event
…ld, make alias mandatory
There was a problem hiding this comment.
Looks like nice work Alex but I left some comments. Some of the comments are more minor and are just suggestions where you can use your own judgement if you want to change or fix or not. Some are more important, here is a summary of the important stuff
- Why renaming/changing existing proto fields? Stuff should be reserved/depcrecated. Does it even work in migration as is?
- Can we dedupe proto between update and create? extract a common type
- Validation of strings which are used in keys. E.g. alias not validated
- Please use gerrc libray and errorsmod.Wrap to define errors, and use errors.Join to combine them , there are examples in the code already https://pkg.go.dev/github.com/dymensionxyz/gerr-cosmos@v1.0.0/gerrc. Since error handling is like 50% of go code it's important to do it well
- regex compilation
- warn/document expensive methods
Also please dont name stuff checkIfCan.. , just do can..
app/upgrades/v4/upgrade_test.go
Outdated
| oldRollapps = make([]types.Rollapp, nRollapps) | ||
| newRollapps = make([]rollapptypes.Rollapp, nRollapps) | ||
|
|
||
| for i := 0; i < nRollapps; i++ { |
Co-authored-by: Michael Tsitrin <michael@dymension.xyz>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
x/rollapp/keeper/rollapp.go
Outdated
| func (k Keeper) checkIfRollappExists(ctx sdk.Context, id, alias string) error { | ||
| rollappId, err := types.NewChainID(id) | ||
| if err != nil { |
There was a problem hiding this comment.
+1. I'd say it's more of a validation of params than checking if one exists. cause if it exists and frozen it may still be valid.
| // registration_fee is the fee that is required to register a rollapp | ||
| cosmos.base.v1beta1.Coin registration_fee = 2 | ||
| [ (gogoproto.nullable) = false, (gogoproto.moretags) = "yaml:\"registration_fee\"" ]; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #980 +/- ##
==========================================
- Coverage 28.42% 26.52% -1.91%
==========================================
Files 253 304 +51
Lines 33786 48934 +15148
==========================================
+ Hits 9605 12980 +3375
- Misses 22820 33932 +11112
- Partials 1361 2022 +661 ☔ View full report in Codecov by Sentry. |
|
tests are failing cause we still don't support the new rollapp registration flow in the e2e tests. |
Description
Closes #978
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...
Unreleasedsection inCHANGELOG.mdgodoccommentsSDK Checklist
maptime.Now()sendCoinand notSendCoinsFull security checklist here
----;
For Reviewer:
---;
After reviewer approval: