feat(genesis bridge): genesis transfers #449
Conversation
This reverts commit a6124a4.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/hub-genesis/keeper/ibc_module.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/hub-genesis/keeper/ibc_module.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/hub-genesis/keeper/ibc_module.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/hub-genesis/keeper/ibc_module.go
|
Thanks @srene |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/hub-genesis/types/state.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/hub-genesis/types/state.go
| @@ -1,6 +1,7 @@ | |||
| package keeper_test | |||
|
|
|||
| import ( | |||
| _ "embed" | |||
| func (w IBCModule) mintAndTransfer( | ||
| ctx sdk.Context, | ||
| n int, | ||
| a types.GenesisAccount, |
| l.Info("Sent genesis transfer.", "index", i, "receiver", a.GetAddress(), "coin", a) | ||
| } | ||
|
|
||
| state.GenesisAccounts = nil |
There was a problem hiding this comment.
don't u prefer to have additional bool flag?
now when querying the hub-genesis state it seems as no allocation were registered
| channelID string, | ||
| ) error { | ||
| coin := a.GetAmount() | ||
| err := w.mintCoins(ctx, types.ModuleName, sdk.Coins{coin}) |
There was a problem hiding this comment.
what about validating the minted denom is the actual native denom of the RA?
otherwise u can mint whatever
There was a problem hiding this comment.
shouldn't we also support rollapps operating with exclusively 3rd party tokens?
There was a problem hiding this comment.
I don't have an opinion on this
Last I spoke to Omri on the product perspective, the rollapp can only hurt himself here, so it's fine
cc (@omritoptix wdyt? )
There was a problem hiding this comment.
As I see it, @mtsitrin is correct in the fact that you could potentially fake denoms on the rollapp, i.e set the coin to channel-0/adym which would than mint it on the rollapp, however in terms of implications, I believe it will fail the transfer, as on the hub it will try to unescrow adym from the vault (i.e ibc channel of this rollapp), and since adym was never escrowed (as the bridge is only open after this phase is completed successfully) it will not be able to complete the transfer (ackErr) and the bridge will forever be closed.
Assuming what I'm saying is correct, I think we can skip the validation on the security level. However with that being said, I think we can easily add a validation on the hub-genesis/keeper.go/genesis.go when importing the genesis accounts that the denom is indeed a valid non-ibc denom (e.g only alpha numeric letter).
|
merged in the middle of review |
PR Standards
See
https://github.com/dymensionxyz/research/issues/278 for the master issue.
There are a few outstanding items, that this PR does not try to solve.
This PR
Opening a pull request should be able to meet the following requirements
Closes #448
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.
For Author:
godoccommentsFor Reviewer:
After reviewer approval:
Summary by CodeRabbit
New Features
IBCModulefor handling channel opening confirmations and genesis account transfers.Improvements
Statevalidation logic with specific checks forAmountandAddress.Refactor