Add fields for syncedBlock and leaves in EVMChainStorage class#106
Add fields for syncedBlock and leaves in EVMChainStorage class#106nepoche wants to merge 6 commits into
Conversation
nepoche
force-pushed
the
add-beresheet-mixers
branch
from
44a3466 to
4e87984
Compare
|
@nepoche are we mergin this or it was just a plaground? |
|
We can use these places to discuss both options IMO. If we agree we like
the proposal (and any changes) then it’s worth merging. I’ll aim to review
this tomorrow.
…On Tue, Jun 22, 2021 at 4:36 PM Ahmed Korim ***@***.***> wrote:
@nepoche <https://github.com/nepoche> are we mergin this or it was just a
plaground?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#106 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADELLF4SQ6WNCNXF5TPQ6ADTUDX4JANCNFSM47EEXE4Q>
.
|
|
I suggest renaming these files too tbh. Also, we're mixing camel case named files I vote camelcasing. My suggestion is:
|
| gasLimit: 6000000, | ||
| gasPrice: utils.toWei('1', 'gwei'), | ||
| value: '0x16345785D8A0000', | ||
| value: await this.denomination, |
There was a problem hiding this comment.
Maybe I don't know much about getters but why are we awaiting this?
There was a problem hiding this comment.
This makes a call to the RPC to read the denomination from the contract deployed on the evm. We need to wait for the RPC response.
There was a problem hiding this comment.
Awesome, so this is mostly to generalise over Substrate chains and EVM, since we get denomination info often asynchronously. Good stuff!
| } | ||
| const address = JSON.parse(data).contractsAddresses; | ||
| return { | ||
| nativeAnchor: new EvmChainStorage(address), |
There was a problem hiding this comment.
Is this currently a nativeAnchor because the only mixers we currently have supported in the dApp are native? I suppose we'll want to plan accordingly for ERC20 supported mixers. Might be worth updating this now, thoughts?
| size: 0.1, | ||
| address: '0x876eCe69618e8E8dd743250B036785813824D2D7', | ||
| symbol: 'ETH', | ||
| syncedBlock: 120000, // should be hardcoded to deployed block number |
There was a problem hiding this comment.
I still don't know what syncedBlock means. This should err on the side of being as verbose as needed. Is this the latest synced block? If so, lets call it, latestSyncedBlock or lastSyncedBlock or lastQueriedBlock, etc.
I think we should also add another parameter for createdAtBlock to indicate the block at which these mixers were created. This way, for new clients, we can begin fetching events immediately from the point they were created rather than before ever, mistakenly
There was a problem hiding this comment.
I agree the field can be named something more descriptive like lastQueriedBlock.
I think that these EvmChainStorage objects will be passed default values as params to the constructor. Therefore, the lastQueriedBlock can pass the deployed block number, and new clients will only ever start querying from the deployed block. In the constructor, we can read from the localStorage to determine if the client has anything in storage for the particular mixer.
| @@ -0,0 +1,22 @@ | |||
| import { MixerSize } from '@webb-dapp/react-environment'; | |||
|
|
|||
| export class EvmChainStorage { | |||
There was a problem hiding this comment.
I think this class should be renamed to EvmChainMixerInfo. This is because most of the fields inside of this class should not be dealing with storage.
3 participants
I'm trying to determine: