BlockHeaderRegistry #123
Open
BlockHeaderRegistry #123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@leonprou I believe I'm supposed to sync up with you to figure out exactly how to structure the runtime logic for the changes. Everything works here, I just need to know exactly where you guys want to go with this feature. For example, who can submit signatures for which blocks (are cycles important), can validators submit signatures for multiple blocks at the same height, how many blocks should be confirmed before submitting the signatures, etc. |
sirpy
suggested changes
May 2, 2022
There was a problem hiding this comment.
- contracts should not be here, they are irrelevant to the fuseapp
- what are all the changes to fuseapp files not related to the bridge?
- you are missing unit tests for the fuseapp bridge part, it is part of the bounty requirements
By default fuse+ethereum+bsc are always enabled no matter if they are in the list or not
@pmlambert you missed the comment above You are still not handling errors correctly in the .subscribe, better extract the whole .subscribe callback to a function |
sirpy
suggested changes
May 9, 2022
app/index.js
Outdated
| logger.info('emitRegistry') | ||
| const currentBlock = (await web3.eth.getBlockNumber()).toNumber() | ||
| const chains = await blockRegistry.methods.getRpcs.call() | ||
| await Promise.all(chains.filter(chain => !blockchains[chain[0]] || blockchains[chain[0]].rpc != chain[1]).map(async (chain) => initBlockchain(...chain))) |
There was a problem hiding this comment.
if one initblockchain fail it shouldnt fail the whole function
app/index.js
Outdated
Comment on lines
51
to
60
| if (chainId == 122) { | ||
| let cycleEnd = (await consensus.methods.getCurrentCycleEndBlock.call()).toNumber() | ||
| let validators = await consensus.methods.currentValidators().call() | ||
| const numValidators = validators.length | ||
| blockchains[chainId].blocks[block.hash] = await signFuse(block, chainId, blockchain.provider, blockchain.signer, cycleEnd, validators) | ||
| } | ||
| else { | ||
| blockchains[chainId].blocks[block.hash] = await sign(block, chainId, blockchain.provider, blockchain.signer) | ||
| } | ||
| }) |
There was a problem hiding this comment.
handle exceptions and extract to a function
|
Every step has its own requirement for unit tests. you can check the bounty and check boxes again for each step. |
sirpy
suggested changes
May 9, 2022
app/index.js
Outdated
| logger.info(`initBlockRegistryContract`, process.env.BLOCK_REGISTRY_ADDRESS) | ||
| blockRegistry = new web3.eth.Contract(require(path.join(cwd, 'abi/blockRegistry')), process.env.BLOCK_REGISTRY_ADDRESS) | ||
| initBlockchain(1, process.env.ETH_RPC || throw "Missing ETH_RPC in environment") | ||
| initBlockchain(56, process.env.BSC_RPC || throw "Missing BSC_RPC in environment")) |
app/index.js
Outdated
| function initBlockRegistryContract() { | ||
| logger.info(`initBlockRegistryContract`, process.env.BLOCK_REGISTRY_ADDRESS) | ||
| blockRegistry = new web3.eth.Contract(require(path.join(cwd, 'abi/blockRegistry')), process.env.BLOCK_REGISTRY_ADDRESS) | ||
| initBlockchain(1, process.env.ETH_RPC || throw "Missing ETH_RPC in environment") |
app/index.js
Outdated
| logger.info('emitRegistry') | ||
| const currentBlock = (await web3.eth.getBlockNumber()).toNumber() | ||
| const chains = await blockRegistry.methods.getRpcs.call() | ||
| await Promise.all(chains.filter(chain => !blockchains[chain[0]] || blockchains[chain[0]].rpc != chain[1]).map(async (chain) => initBlockchain(...chain))) |
app/index.js
Outdated
| }) | ||
| }) | ||
| } catch(e) { | ||
| throw `emitRegistry failed trying to update rpcs` |
There was a problem hiding this comment.
i dont think that should prevent from calling addSignedBlocks part
app/index.js
Outdated
| const isValidator = await consensus.methods.isValidator(web3.utils.toChecksumAddress(account)).call() | ||
| if (!isValidator) { | ||
| logger.warn(`${account} is not a validator, skipping`) | ||
| return | ||
| } | ||
| await emitInitiateChange() | ||
| await emitRewardedOnCycle() | ||
| await emitRegistry() |
There was a problem hiding this comment.
i dont think an exception here should cause the fuseapp to exit
sirpy
suggested changes
May 9, 2022
app/index.js
Outdated
| try { | ||
| logger.info('emitRegistry') | ||
| const currentBlock = (await web3.eth.getBlockNumber()).toNumber() | ||
| const chains = await blockRegistry.methods.getRpcs.call() |
There was a problem hiding this comment.
should be getRpcs()
and that's why I want unit tests or testnet deployment with mock contracts to see that this whole process is working
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.