petrovska-petro / bribes_module_speed

0 stars 0 forks source link

explicitly test successful signing of cowswap order #1

Open gosuto-inzasheru opened 2 years ago

gosuto-inzasheru commented 2 years ago

https://github.com/petrovska-petro/bribes_module_speed/blob/e32551c5452dc38914c0372ef39d644039b957e1/tests/test_bribe_swap_for_weth.py#L145-L149

does IGnosisSafe(safe).execTransactionFromModule return something on successful execution?

i guess the require here assures that it runs successfully,

https://github.com/petrovska-petro/bribes_module_speed/blob/e32551c5452dc38914c0372ef39d644039b957e1/contracts/SpeedScopedModule.sol#L138-L146

but an explicit test would be nice maybe.

(it is a tough situation, because the signature happens on fork and therefor never changes the api state)

petrovska-petro commented 2 years ago

https://github.com/petrovska-petro/bribes_module_speed/blob/e32551c5452dc38914c0372ef39d644039b957e1/tests/test_bribe_swap_for_weth.py#L145-L149

does IGnosisSafe(safe).execTransactionFromModule return something on successful execution?

i guess the require here assures that it runs successfully,

https://github.com/petrovska-petro/bribes_module_speed/blob/e32551c5452dc38914c0372ef39d644039b957e1/contracts/SpeedScopedModule.sol#L138-L146

but an explicit test would be nice maybe.

(it is a tough situation, because the signature happens on fork and therefor never changes the api state)

i could add test that ensure that specific order uid has being signed properly in the settlement contract at least, good call.

yep, the require ensures proper exec on-chain, no reverts occured etc, since it returns a bool if all good should be true.

petrovska-petro commented 2 years ago

pushed signature assertion on this commit

@jorijnsmit, for further feedback better to consolidate feedback over here