palomachain / paloma

The fast blockchain messenger protocol

Apache License 2.0

291 stars 135 forks source link

chore: gas estimates in sign bytes #1257

Closed maharifu closed 3 months ago

maharifu commented 3 months ago

Background

Include gas estimate and fee structure in turnstone messages hash.

Testing completed

[ ] test coverage exists or has been added/updated
[ ] tested in a private testnet

Breaking changes

[ ] I have checked my code for breaking changes
[ ] If there are breaking changes, there is a supporting migration.

maharifu commented 3 months ago

Great work, only some small remarks. There is a panic as well where there shouldn't be.

Regarding the panics, we already have other panics in the same functions, I just continued the same style. I think we can refactor them out and return an error instead. However, I don't see these functions being used in Begin or End blockers. Is this an issue or a false positive?

byte-bandit commented 3 months ago

Great work, only some small remarks. There is a panic as well where there shouldn't be.

Regarding the panics, we already have other panics in the same functions, I just continued the same style. I think we can refactor them out and return an error instead. However, I don't see these functions being used in Begin or End blockers. Is this an issue or a false positive?

Unless I'm mistaken, the panic statements are in hot code paths. Did a short scan and found one in the keccak256 for submit logic call and valset update for example, which are currently both used to calculate the bytes to sign.

It's interesting the CodeQL plugin didn't find these yet, but we definitely want them out. In theory, failing to pack the arguments should never happen, but better be safe then risk an app hash.

maharifu commented 3 months ago

Great work, only some small remarks. There is a panic as well where there shouldn't be.

Regarding the panics, we already have other panics in the same functions, I just continued the same style. I think we can refactor them out and return an error instead. However, I don't see these functions being used in Begin or End blockers. Is this an issue or a false positive?

Unless I'm mistaken, the panic statements are in hot code paths. Did a short scan and found one in the keccak256 for submit logic call and valset update for example, which are currently both used to calculate the bytes to sign.

It's interesting the CodeQL plugin didn't find these yet, but we definitely want them out. In theory, failing to pack the arguments should never happen, but better be safe then risk an app hash.

You are right! I'll refactor that out and return an error instead.