imp: simplified BuildMerklePath
#7863
Conversation
| func BuildMerklePath(prefix [][]byte, path []byte) commitmenttypesv2.MerklePath { | ||
| prefixLength := len(prefix) | ||
| if prefixLength == 0 { | ||
| panic("cannot build merkle path with empty prefix") |
There was a problem hiding this comment.
Don't think this should necessarily error. If there's no prefix, the path should just be returned as it was originally. See removed diff
This would be equivalent to a chain storing the ICS24 paths directly under root. Which is potentially possible for a solomachine chain or something equivalent
There was a problem hiding this comment.
What is the reasoning for changing the behavior here?
There was a problem hiding this comment.
Ahh i see it was decided in the original issue! Ok, i mean i suppose we could require all implementations to store under a prefix
There was a problem hiding this comment.
I don't remember the entire context for why we said this, but it was something we mentioned during the alpha audit itself, I believe.
There was a problem hiding this comment.
I could take this back. I don't feel strongly about it. What should we do?
There was a problem hiding this comment.
@AdityaSripal can have the final say on it :)
There was a problem hiding this comment.
I think its more likely that someone accidentally leaves prefix empty than having a counterparty that is genuinely storing everything under root.
So lets have the error for now, and if we ever have a user run into this issue and report it we can change the behavior of this function quite easily
|
|
||
| lastElement := prefixKeys[len(prefixKeys)-1] | ||
| // copy prefix to avoid modifying the original slice | ||
| prefix = append([][]byte(nil), prefix...) |
There was a problem hiding this comment.
Should we name this var to something else since it eventually becomes the full path?
There was a problem hiding this comment.
Agreed.
Also good to not reassign or change function arguments, so would prefer a new name in any case.
|
Description
closes: #7642
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.