Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ADR 001: coin cross-chain transfer source tracing #6662
ADR 001: coin cross-chain transfer source tracing #6662
Changes from 4 commits
0833c55
6bca8df
13cf126
ca1de7e
3a9dc3e
5532428
82a30db
5abb1ed
a4fdab9
5ba6b58
2f69f4d
0dab556
7ebf21b
56d2b11
3ff4bb9
fbaf655
1954510
95199d5
38d99eb
d63f0c9
628ec72
33ba62e
80d76b0
8591fc3
1e8f02f
ae34c2c
4da7e85
18bc6ec
1560d6d
ed677ec
8e5c044
6d3b25f
152b2e1
e9ccb3c
295d93c
f35a843
54ee703
e66e0e3
dd564e8
a7f99d0
52b621b
ae3b4b3
b73908d
6fd5c19
810d517
a18409e
cdc1d66
88994bb
d3de242
f886641
e8d754f
c205a8d
877d67f
cadeacf
3a69288
a71be5c
7b882c4
226eb92
c0cab23
ea3655c
9a9b144
75a3eff
59363c1
18e86c1
2dcfff2
07b9ec0
cb4907f
c01d0da
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the elaborate and well throughout ADR @fedekunze, but I can see that I'll already have concerns with this because we're conflating concerns and use-cases w/ IBC. Specifically, I do not believe
trace
should be a first-class citizen field ofCoin
. Please consider instead using some notion of metadata.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should use metadata and not add this to
Coin
itself. @fedekunze I have some ideas on how to address this more generally and will write something up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I realize what I have in mind will be a larger ADR that also covers supply. I'm hoping to get to that relatively soon as these are things I've thought about for some of Regen's use case which allows for some modules to arbitrarily create approved denom's.
Anyway the gist of it is that from the perspective of the bank module, IBC transfer is just minting or burning coins for some denom which it has the right to manage.
I propose we use prefixes for these denoms such as
ibc:
oribc/
and then IBC assigns an auto-generated ID that looks something likeibc:2g8sd6h
. Or maybeibc:atom:2g8sd6h
to be more descriptive if the source denom isatom
.Then the
Trace
information is something that gets managed as metadata by the ibc transfer module. So the ibc transfer keeper would then have a method likeDescribeIBCDenom(denom string) IBCDenomMetadata
to retrieve theTrace
data.One problem that I pointed out in the call is that balances are indexed by
denom
. So addingTrace
would break that in addition polluting any balances with all of that metadata. Instead we can just treatdenom
s as dumb identifiers which are fully described by their metadata.To take it a step further, I would restrict which modules can mint/burn which denoms. So
x/mint
would get rights toatom
,x/ibc-transfer
would mange the wholeibc:*
prefix, etc. But that gets into supply which I'd love to address too, but later after stargate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense, although for now (IBC 1.0) I think we should do the simplest thing which solves our main problem, and hash the denominations in the
20-transfer
module to allow for arbitrary port/channel paths, and keep a lookup table so that we can lookup the port(s) & channel(s) from the denomination.