[Stubbed] Greg/startup #71
Conversation
|
|
| // Listen for Eth1Deposit logs | ||
| provider.on(depositFilter, (previousReceiptRoot: hash32[], data: DepositData, totalDepositcount: uint64, log) => { | ||
| const newDeposit: Deposit = formatDeposit(previousReceiptRoot, data, totalDepositcount); | ||
| deposits.push(newDeposit); |
There was a problem hiding this comment.
Whats the point of storing the deposits here? Should we maybe provide a function to allow their retrieval instead of discarding them in this code block?
There was a problem hiding this comment.
Not sure what you mean by discarding...
This is where classes could be very useful, and we can store to a state variable. But the objective here is we need to retrieve the deposits until chainstart emits then we take the formatted deposit[] and pass it to the next function in start to generate initial state
There was a problem hiding this comment.
Correct me if I'm wrong, deposits will only added until the ChainStart event resolves the promise. ie. future deposits won't be captured here. It does seem a class structure would be a way to resolve this.
There was a problem hiding this comment.
Correct, that's why I created the deposits helper. Um unsure with the current spec at what point a new deposit needs to be captured. I.e what point in the state transition.
| * intial state data. | ||
| * @returns {Promise<ChainStart>} | ||
| */ | ||
| const waitForChainStart = () => { |
There was a problem hiding this comment.
I think its plausible to test this functionality at this point, the notion of waiting for the ChainStart log isn't going to change. Please add tests if you agree.
There was a problem hiding this comment.
You know you're actually right. I'll probably have to import the vyper contract? Unless #66 gets closed
There was a problem hiding this comment.
After some thought, you're right I can test this. The tests will only cover the actual chainstart functionality and not the deposits for now.
There was a problem hiding this comment.
Do we want to add truffle/embark? Before continuing we'll need to discuss if we want a framework for testing smart contracts or not
There was a problem hiding this comment.
What about etherlime? It uses ethers.
There was a problem hiding this comment.
etherlime looks good. Still need to figure out what we want to do. Ultimately, we could do it manually.
|
|
||
| // Smart contract addresses | ||
| export const MAINNET_DEPOSIT_ADDRESS = "0x0"; // Address for teh deposit function on ethereum. | ||
| export const DEPOSIT_CONTRACT_BLOCK_NUMBER = 5222222; |
There was a problem hiding this comment.
Can you explain the use case for this?
There was a problem hiding this comment.
It actually should be stubbed to "tbd". But it's the block at which the contract is deployed, so we can start looking for receipts at that block number, and then work forward.
Will add comments
There was a problem hiding this comment.
Isn't that what the ChainStart event is for? As I understand it deposits made after are still valid so I don't see the point in differentiating those made before and from those made after.
There was a problem hiding this comment.
MMM good call. Pretty sure I added this earlier in the implementation
There was a problem hiding this comment.
Slight confusion, the ChainStart event is not for this. This is to reset to the block at which the contract is created at. This way we can begin looking at all deposits up to the ChainStart event.
As mentioned before, perhaps we need to re-org this a bit.
|
Closing per #76 -- May use for spare parts later on |
NOTE :: Most functionality is stubbed, but closely resembles what the end state should be. This will help to open up more functionalities, specifically surrounding state transitioning.
I began creating the start functionality. Currently, I have a stubbed
startmethod that polls the eth1.x validator deposit contract. Once theChainStartlog is emitted a promise is returned to begin the rest of the beacon chain functionality.Caveats (outside of stubbed functionality):
start(), this doesn't really make sense, but perhaps a.then()could suffice.