search

omni / poa-bridge

POA <-> Ethereum bridge for self transfers of POA native token to POA20 (ERC20 representation). Not supported. Use TokenBridge instead
https://github.com/poanetwork/token-bridge
GNU General Public License v3.0
79 stars 38 forks source link

Problem: required_signatures is static #82

Closed yrashk closed 6 years ago

yrashk commented 6 years ago

Validators' information is completely configured through validators contracts and does not depend on authorities.required_signatures parameter of bridge's configuration.

The number of validators also could be changed during run-time and therefore authorities.required_signatures parameter will not reflect the actual number of signatures required for transaction validation.

Solution: retrieve required_signatures from RequiredSignaturesChanged event and requiredSignatures() method

Closes #74

akolotov commented 6 years ago

@yrashk could you please clarify a bit the method you are using? I went through the delta and, think, got quite confused.

akolotov commented 6 years ago

@yrashk @rstormsf do you think the set of changes could be reduced and logic will be more straightforward if we change CollectedSignatures event to:

CollectedSignatures(address authorityResponsibleForRelay, bytes32 messageHash, uint256 NumberOfCollectedSignatures);

So, in this case we don't need to watch for RequiredSignaturesChanged() event and don't need to call requiredSignatures() through RPC at all.

yrashk commented 6 years ago

@akolotov do you mean CollectedSignatures(address authorityResponsibleForRelay, bytes32 messageHash, uint256 NumberOfRequiredSignatures) ?

yrashk commented 6 years ago

I think this will greatly simplify bridge's code. Any thoughts on potential ways this can go wrong, though? Just to be on the safe side.

rstormsf commented 6 years ago

I don't see any problems

akolotov commented 6 years ago

@yrashk yes. This event was extended today under https://github.com/poanetwork/poa-bridge-contracts/pull/41.

https://github.com/poanetwork/poa-bridge-contracts/blob/aa811e7223847320dd83ce0a8a46d3bcad68dc8b/contracts/upgradeable_contracts/U_ForeignBridge.sol#L19

and

https://github.com/poanetwork/poa-bridge-contracts/blob/aa811e7223847320dd83ce0a8a46d3bcad68dc8b/contracts/upgradeable_contracts/U_ForeignBridge.sol#L184-L187

yrashk commented 6 years ago

I updated this PR to use this new functionality. It has simplified this part of the bridge significantly!

yrashk commented 6 years ago

master with #93 has been tagged as v0.2.1

akolotov commented 6 years ago

@yrashk could you confirm that these changes has no conflicts and can be committed to master?

yrashk commented 6 years ago

@akolotov Yes, it has no conflicts and passes tests when attempted to merge into master (I just verified)

akolotov commented 6 years ago

Thank you! I will merge this PR in this case.