Use plain strings as chain IDs#432
Conversation
also exports a sha256_hash utility
uses sha256 utility from sequencer-validation, removing its own utility
github-actions
Bot
added
conductor
SuperFluffy
changed the title
| let rollup_data_tree = MerkleTree::from_leaves(self.rollup_txs.clone()); | ||
| let rollup_data_root = rollup_data_tree.root(); | ||
| let mut leaf = self.chain_id.as_ref().to_vec(); | ||
| let mut leaf = utils::sha256_hash(self.chain_id.as_bytes()).to_vec(); |
There was a problem hiding this comment.
does this PR change the format of the sequence::Action merkle tree? can you update the doc in specs/sequencer-inclusion-proofs.md accordingly?
| data.push(action.data.clone()); | ||
| }) | ||
| .or_insert_with(|| vec![action.data.clone()]); | ||
| // FIXME(https://): change sequence actions to carry string chain IDs |
| }) | ||
| .or_insert_with(|| vec![action.data.clone()]); | ||
| // FIXME(https://): change sequence actions to carry string chain IDs | ||
| let chain_id = String::from_utf8_lossy(&action.chain_id); |
There was a problem hiding this comment.
is it better to us from_utf8_lossy over from_utf8?
There was a problem hiding this comment.
The problem is that we don't know what's inside action.chain_id. Right now we define it as bytes (in the protobuf definition), so it could be bytes that are not utf8. I guess we could return an error instead.
noot
left a comment
noot
left a comment
There was a problem hiding this comment.
looks fine, please update specs/sequencer-inclusion-proofs.md before merging!
|
@noot I have pushed a small modification where the |
SuperFluffy
force-pushed
the
superfluffy/chain-id-string
branch
from
4b980b4 to
3dd1c45
Compare
github-actions
Bot
added
proto
sambukowski
left a comment
sambukowski
left a comment
There was a problem hiding this comment.
lgtm, just need to fix build tests
|
Closing this in favor of #436 |
3 participants
Summary
Removed
ChainIdtype in favor of a plain old String. Strings are hashed before constructing commitments.Background
#395 introduces plain strings as chain IDs in its protobuf definitions. This patch is a step to make the refactor easier.
Open question
SequenceActionprotobuf message still takes bytes as the chain ID. It should also be changed to take aStringChanges
ChainIdtype by a plainStringStringusing sha256 before constructing merkle trees and roots as commitmentsTesting
All present tests should still pass after this change.
Breaking Changelist
Related Issues
This is a follow-up to #346
Closes #433