-
Notifications
You must be signed in to change notification settings - Fork 214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ALM snapshots #382
Add ALM snapshots #382
Conversation
# Conflicts: # alm/src/hooks/useBlockConfirmations.ts # alm/src/state/StateProvider.tsx # alm/src/utils/contract.ts
alm/src/utils/contract.ts
Outdated
@@ -19,33 +33,58 @@ export const getRequiredBlockConfirmations = async (contract: Contract, blockNum | |||
return parseInt(blockConfirmations) | |||
} | |||
|
|||
export const getValidatorAddress = (contract: Contract) => contract.methods.validatorContract().call() | |||
export const getValidatorAddress = async (contract: Contract, snapshotProvider: SnapshotProvider) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validator contract address could be changed later so data in the snapshot could be outdated. There is no big advantage if we get them form the snapshot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which scenario do you think the validator contract address could be changed? Validator contracts are usually a proxy too and the bridge contract does not provide a method for such functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine that the bridge has lost the control over the validators contract e.g. the validators contract upgradability admin is not the same account as the bridge upgradability admin AND the private key of the validators contract upgradability admin was lost. So, we will need to stop the bridge and deploy another validator contract with the same parameters as an old one except the validators contract upgradability admin account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the example!
In that case, as you suggested it makes sense to remove the validator contract address.
If the validator contract address is changed, right now there is no way to get the historic values of the validator contracts addresses. So if a user introduces a transaction that was previous to the change to a new validator contract, we won't be able to get the correct parameters of validator list and required signatures at that moment since we won't be able to get the validator contract of that moment and in the new contract, validators could be different.
Does it make sense to add a new event like ValidatorContractChanged
when the bridge is initialized? it could be emitted also when the validator contract changes in some upgrade method invoked. Then in the future, ALM could use those events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if a user introduces a transaction that was previous to the change to a new validator contract, we won't be able to get the correct parameters of validator list and required signatures at that moment since we won't be able to get the validator contract of that moment and in the new contract, validators could be different.
Does it make sense to add a new event like ValidatorContractChanged when the bridge is initialized? it could be emitted also when the validator contract changes in some upgrade method invoked. Then in the future, ALM could use those events.
OK. Let's introduce such event in the contracts. But we will not handle this situation in ALM so far since it is too rare.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the validator address from the snapshot 33c06ee
And also created an issue for the event in the contracts omni/tokenbridge-contracts#445
Closes #370
Add a script that runs before the app is built. The script will get useful information about the bridge contracts and networks and store it in the sources of the app to reduce the number of requests needed when the user visits a page.
Another fix introduced here is to set a fixed size for the columns in the tables for validators confirmations and execution in 072cd9d. Previously, the column adjusted the size based on the content provided which caused the columns to move and not be correctly aligned.
Updated the demo: https://alm-demo-e0998.web.app/