MonitoringService.monitor() takes a MessageTypeId #1025
Conversation
pirapira
force-pushed
the
monitor-message-id
branch
from
09f4c4e to
add1794
Compare
Codecov Report
@@ Coverage Diff @@
## master #1025 +/- ##
==========================================
+ Coverage 85.14% 85.15% +0.01%
==========================================
Files 20 20
Lines 1373 1374 +1
Branches 182 182
==========================================
+ Hits 1169 1170 +1
Misses 158 158
Partials 46 46
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1025 +/- ##
==========================================
+ Coverage 85.18% 85.29% +0.11%
==========================================
Files 20 20
Lines 1377 1374 -3
Branches 182 178 -4
==========================================
- Hits 1173 1172 -1
+ Misses 158 157 -1
+ Partials 46 45 -1
Continue to review full report at Codecov.
|
| "\x19Ethereum Signed Message:\n200", | ||
| monitoring_service_address, | ||
| chain_id, | ||
| uint256(MessageTypeId.MSReward), |
There was a problem hiding this comment.
If we add a message id to the signature, shouldn't we put it in the same place for all messages (e.g. first in the signature)? Otherwise there could still be conflicts (although highly unlikely). Example:
Message1 (of MessageTypeId 5):
- 10: TokenAmount
- 5: MessageTypeId
- signature
Message2 (of MessageTypeId 10):
- 10: MessageTypeId
- 5: ChannelID
- signature
There was a problem hiding this comment.
Yes, we should put the ID in the same place always.
In TokenNetwork, the message ID is always the third element after an address and a chain_id.
bytes32 message_hash = keccak256(abi.encodePacked(
signature_prefix,
message_length,
address(this),
chain_id,
uint256(MessageTypeId.Withdraw),
channel_identifier,
participant,
total_withdraw
));
I can change them as well, but that causes some additional work (adjusting the client).
There was a problem hiding this comment.
I didn't expect the third place to be the fixed location. This also required the same length of the first two arguments in all cases to be safe. But I guess this is the case for us. No need to change if you watch out for this.
There was a problem hiding this comment.
I'll add the convention in the contribution guide. I think the current convention is
address message_destination,
uint256 chain_id,
uin256 message_type,
...
I agree the third position looks like a random choice.
| @@ -342,11 +344,13 @@ contract MonitoringService is Utils { | |||
| returns (address signature_address) | |||
| { | |||
| bytes32 message_hash = keccak256(abi.encodePacked( | |||
| "\x19Ethereum Signed Message:\n148", | |||
| "\x19Ethereum Signed Message:\n200", | |||
| monitoring_service_address, | |||
There was a problem hiding this comment.
The reward is not bound to a specific monitoring service. Did you want to use monitoring_service_contract_address here?
There was a problem hiding this comment.
Ah, suddenly I remember that there should be a competition between MSs.
And yes, monitoring_service_contract_address is the right one.
into account when validating the signature.
pirapira
force-pushed
the
monitor-message-id
branch
from
e9e83f0 to
5929723
Compare
|
@karlb please have another look. |
|
@karlb ready for a merge? |
into account when validating the signature.
This closes #1021 .