dymensionxyz / dymension-rdk

Framework for building highly scalable RollApps

Apache License 2.0

98 stars 54 forks source link

feat(denommetadata): inject denom metadata to ibc transfers from RA to Hub #455

Closed zale144 closed 4 months ago

zale144 commented 4 months ago

PR Standards

Opening a pull request should be able to meet the following requirements

Closes #454

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:

[ ] Targeted PR against correct branch
[ ] included the correct type prefix in the PR title
[ ] Linked to Github issue with discussion and accepted design
[ ] Targets only one github issue
[ ] Wrote unit and integration tests
[ ] All CI checks have passed
[ ] Added relevant godoc comments

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 targets main branch, PR should be squashed and merged.
[ ] In case PR targets a release branch, PR should be rebased.

zale144 commented 4 months ago

I'm not sure I'm on board with this architecturally.

I think the denom meta data module should just assume there is one hub and keep track of the registered denoms for it by itself

No need for hub keeper

keep track of the registered denoms for it by itself so how do we prevent the rollapp from sending the denom metadata then?

I think I already addressed a similar comment you posted: even though the hub can always be the same, technically it is still possible the hub chain can do a fork, which I believe would make it's chain ID change.

See: https://github.com/dymensionxyz/dymension-rdk/issues/465

danwt commented 4 months ago

so how do we prevent the rollapp from sending the denom metadata then?

WDYM? I'm just saying to store what you currently store on the hub module in the denom meta module

Re https://github.com/dymensionxyz/dymension-rdk/issues/465 ok maybe. IDK tbh I would have to research. Seems like a delicate thing. Can you just write this PR so that it assumes that problem is solved by another component. Otherwise you're solving too many problems at once and stuff is likely to get reworked/changed anyways

Basically just write your code to have minimal assumptions so that the rest of the codebase can evolve around it independently. Here you're placing quite a lot of assumptions on the management of the hub wrt. forks etc, which is out of scope.