feat: Canonical sign payload for foreign chain transactions#1998
feat: Canonical sign payload for foreign chain transactions#1998
Conversation
1901e8c to
498e2b4
Compare
DSharifi
left a comment
There was a problem hiding this comment.
Looks good to me.
One blocker; I don't want expect/unwrap panic for production code that the node depends on.
5c76a1b to
29e2e19
Compare
d6ef49b to
3d3a8f2
Compare
| let mut hasher = sha2::Sha256::new(); | ||
| borsh::BorshSerialize::serialize(self, &mut hasher)?; | ||
| Ok(Hash256(hasher.finalize().into())) |
There was a problem hiding this comment.
I think I already found a good reason to not have this implementation in the interface type in the long run, unless we are generic over the hasher which is also not necessarily desirable.
On the contract, we should not use sha2 crate, and instead use the host function sha256 which near sdk exports near_sdk::env::sha256. for hashing. This will be much cheaper in terms of gas cost usage.
There was a problem hiding this comment.
yeah the use of host functions is something that can grant duplicate code, but we can also skip it by checking if we are in wasm or not, or a feature flag, that would include near_sdk to access host functions.
There was a problem hiding this comment.
Good point. It's not obvious that this is more performant since the host function doesn't take a writer as input. It probably is, but let's save that as a follow-up optimization then.
There was a problem hiding this comment.
But that's a very interesting take on the logic vs data structures discussion. You're right that different users may have different implementations of algorithms etc.
Although I still think it's net-positive to expose a reference implementation. Anyone can implement their own but test it against the reference. But I'm all in favor of moving this to another crate.
02bf552 to
3ae0285
Compare
3ae0285 to
d67138d
Compare
Closes #1992