-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat: ADR-040: ICS-23 proofs for SMT store #10015
Conversation
6faf915
to
d5eea91
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Commenting so the bot won't close - this builds on #9892 and will be un-drafted when that's merged |
83ddee6
to
37559e4
Compare
Codecov Report
@@ Coverage Diff @@
## master #10015 +/- ##
==========================================
- Coverage 65.85% 64.89% -0.96%
==========================================
Files 659 616 -43
Lines 67511 59315 -8196
==========================================
- Hits 44456 38493 -5963
+ Misses 20469 18587 -1882
+ Partials 2586 2235 -351
|
eaddf6b
to
d878af5
Compare
d94d770
to
e37f769
Compare
e37f769
to
ddf9a63
Compare
This depends on the SmtSpec introduced here cosmos/ics23#57, need to update go.mod once that is merged & tagged |
ddf9a63
to
975027e
Compare
There's an unfortunate consequence of how the SMT functions which may or may not be an issue here. The SMT hashes keys to produce a path, so when a proof is created, the resulting req := abci.RequestQuery{
Path: "/store_name/key",
Data: []byte("key_name"),
Prove: true,
}
res := store.Query(req)
// res.Key == req.Data
// but, res.ProofOps.Ops[0].Key == sha256(res.Key) It doesn't seem like there should be major consequences for this as long as code that deals with proofs doesn't expect these fields to match. If nothing outside the SDK looks at them, it should be fine. But if we do want to change it, it can't be changed within the SMT proof spec as is. There is |
d9dcdb9
to
eaf2fa0
Compare
ea5cc99
to
998a94f
Compare
Update: this depends on cosmos/ics23#61 - marking draft until that's merged |
998a94f
to
caa8a54
Compare
All the dependencies in https://github.com/confio/ics23 have been merged, so this is ready for review (it functions independently of #10174 which is also in-flight). |
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.
left one comment to address before merge - one error check is missing.
@@ -111,8 +111,8 @@ func createExistenceProof(data map[string][]byte, key []byte) (*ics23.ExistenceP | |||
return nil, fmt.Errorf("cannot make existence proof if key is not in map") | |||
} | |||
|
|||
_, ics23, _ := sdkmaps.ProofsFromMap(data) | |||
proof := ics23[string(key)] | |||
_, proofs, _ := sdkmaps.ProofsFromMap(data) |
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.
lat return value is an error - we must check it.
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.
it's actually the list of keys in the map:
func ProofsFromMap(m map[string][]byte) ([]byte, map[string]*tmcrypto.Proof, []string) {
does this depends on the latest / not released changes in ibc repo? |
No changes needed for ibc. But ics23 can bump to |
caa8a54
to
127e902
Compare
Description
Implements ICS-23 conformant proofs for the SMT-based KV store and defines the proof spec as part of ADR-040.
Closes: vulcanize#8
Author Checklist
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.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change