Rpc statediffing #10

Merged
telackey merged 14 commits from rpc_statediffing into statediffing 2019-07-31 17:52:06 +00:00
8 changed files with 155 additions and 172 deletions
Showing only changes of commit 371cef7282 - Show all commits
+11 -10
View File
@@ -21,6 +21,7 @@ package statediff
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
import (
"fmt"
"math/big"
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/hexutil"
@@ -34,7 +35,7 @@ import (
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
// Builder interface exposes the method for building a state diff between two blocks
type Builder interface {
BuildStateDiff(oldStateRoot, newStateRoot common.Hash, blockNumber int64, blockHash common.Hash) (StateDiff, error)
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
BuildStateDiff(oldStateRoot, newStateRoot common.Hash, blockNumber *big.Int, blockHash common.Hash) (StateDiff, error)
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
}
type builder struct {
@@ -54,7 +55,7 @@ func NewBuilder(db ethdb.Database, blockChain *core.BlockChain, config Config) B
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
}
// BuildStateDiff builds a StateDiff object from two blocks
func (sdb *builder) BuildStateDiff(oldStateRoot, newStateRoot common.Hash, blockNumber int64, blockHash common.Hash) (StateDiff, error) {
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
func (sdb *builder) BuildStateDiff(oldStateRoot, newStateRoot common.Hash, blockNumber *big.Int, blockHash common.Hash) (StateDiff, error) {
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
// Generate tries for old and new states
sdb.stateCache = sdb.blockChain.StateCache()
oldTrie, err := sdb.stateCache.OpenTrie(oldStateRoot)
2
@@ -230,8 +231,8 @@ func (sdb *builder) collectDiffNodes(a, b trie.NodeIterator) (AccountsMap, error
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
return diffAccounts, nil
}
func (sdb *builder) buildDiffEventual(accounts AccountsMap) (AccountDiffsMap, error) {
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
accountDiffs := make(AccountDiffsMap)
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
func (sdb *builder) buildDiffEventual(accounts AccountsMap) ([]AccountDiff, error) {
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
accountDiffs := make([]AccountDiff, 0)
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
var err error
for _, val := range accounts {
// If account is not nil, we need to process storage diffs
@@ -242,20 +243,20 @@ func (sdb *builder) buildDiffEventual(accounts AccountsMap) (AccountDiffsMap, er
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
return nil, fmt.Errorf("failed building eventual storage diffs for %s\r\nerror: %v", common.BytesToHash(val.RawKey), err)
}
}
accountDiffs[common.BytesToHash(val.RawKey)] = AccountDiff{
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
accountDiffs = append(accountDiffs, AccountDiff{
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
Key: val.RawKey,
Value: val.RawValue,
Proof: val.Proof,
Path: val.Path,
Storage: storageDiffs,
}
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
})
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
}
return accountDiffs, nil
}
func (sdb *builder) buildDiffIncremental(creations AccountsMap, deletions AccountsMap, updatedKeys []string) (AccountDiffsMap, error) {
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
updatedAccounts := make(AccountDiffsMap)
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
func (sdb *builder) buildDiffIncremental(creations AccountsMap, deletions AccountsMap, updatedKeys []string) ([]AccountDiff, error) {
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
updatedAccounts := make([]AccountDiff, 0)
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
var err error
for _, val := range updatedKeys {
hashKey := common.HexToHash(val)
@@ -270,13 +271,13 @@ func (sdb *builder) buildDiffIncremental(creations AccountsMap, deletions Accoun
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
return nil, fmt.Errorf("failed building incremental storage diffs for %s\r\nerror: %v", hashKey.Hex(), err)
}
}
updatedAccounts[common.HexToHash(val)] = AccountDiff{
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
updatedAccounts = append(updatedAccounts, AccountDiff{
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
Key: createdAcc.RawKey,
Value: createdAcc.RawValue,
Proof: createdAcc.Proof,
Path: createdAcc.Path,
Storage: storageDiffs,
}
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
})
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
delete(creations, common.HexToHash(val))
delete(deletions, common.HexToHash(val))
}
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:12:06 +00:00 (Migrated from github.com)
Review

I wonder if we can extract each of these branches in the if/else block into it's own method: collectPathAndProofs, collectOnlyLeaves, etc.

I wonder if we can extract each of these branches in the `if/else` block into it's own method: `collectPathAndProofs`, `collectOnlyLeaves`, etc.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
elizabethengelman commented 2019-05-16 21:18:13 +00:00 (Migrated from github.com)
Review

also wondering if each of these if/else branches could be pulled out into their own methods, so that buildStorageDiffsFromTrie is a bit easier to read from a high level.

also wondering if each of these `if/else` branches could be pulled out into their own methods, so that `buildStorageDiffsFromTrie` is a bit easier to read from a high level.
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)

I've refactored this quite a bit, it's still one method but much more concise. Might still be best to extract into separate methods but I think we would need 4 different methods to cover the different combinations of processing (leafs only, leafs with their proofs and paths, all nodes, all nodes + leaf proof and paths)
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
Review

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.

Did similar refactoring as above, let me know what you think! Separate methods might still be the best option.
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
elizabethengelman commented 2019-07-22 16:29:49 +00:00 (Migrated from github.com)
Review

This method isn't being used anymore, and can probably be deleted.

I think this was being used previously to get the contract's address. But from what I remember the sdb.chainDB.Get call will only return the address with an archive node, which is partially why we're currently not able to get the address,

This method isn't being used anymore, and can probably be deleted. I think this was being used previously to get the contract's address. But from what I remember the `sdb.chainDB.Get` call will only return the address with an archive node, which is partially why we're currently not able to get the address,
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
Review

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...

Thank you for bringing this up! I'll get rid of this, and take a closer look to see if we can do something similar with a full node...
+71 -84
View File
@@ -19,7 +19,7 @@ package statediff_test
import (
"bytes"
"math/big"
"reflect"
"sort"
"testing"
"github.com/ethereum/go-ethereum/common"
@@ -33,8 +33,8 @@ import (
var (
contractLeafKey common.Hash
emptyAccountDiffEventualMap = make(statediff.AccountDiffsMap)
emptyAccountDiffIncrementalMap = make(statediff.AccountDiffsMap)
emptyAccountDiffEventualMap = make([]statediff.AccountDiff, 0)
emptyAccountDiffIncrementalMap = make([]statediff.AccountDiff, 0)
block0Hash, block1Hash, block2Hash, block3Hash common.Hash
block0, block1, block2, block3 *types.Block
builder statediff.Builder
@@ -65,7 +65,7 @@ func TestBuilder(t *testing.T) {
type arguments struct {
oldStateRoot common.Hash
newStateRoot common.Hash
blockNumber int64
blockNumber *big.Int
blockHash common.Hash
}
@@ -166,11 +166,11 @@ func TestBuilder(t *testing.T) {
arguments{
oldStateRoot: block0.Root(),
newStateRoot: block0.Root(),
blockNumber: block0.Number().Int64(),
blockNumber: block0.Number(),
blockHash: block0Hash,
},
&statediff.StateDiff{
BlockNumber: block0.Number().Int64(),
BlockNumber: block0.Number(),
BlockHash: block0Hash,
CreatedAccounts: emptyAccountDiffEventualMap,
DeletedAccounts: emptyAccountDiffEventualMap,
@@ -183,22 +183,14 @@ func TestBuilder(t *testing.T) {
arguments{
oldStateRoot: block0.Root(),
newStateRoot: block1.Root(),
blockNumber: block1.Number().Int64(),
blockNumber: block1.Number(),
blockHash: block1Hash,
},
&statediff.StateDiff{
BlockNumber: block1.Number().Int64(),
BlockNumber: block1.Number(),
BlockHash: block1.Hash(),
CreatedAccounts: statediff.AccountDiffsMap{
testhelpers.Account1LeafKey: {
Key: testhelpers.Account1LeafKey.Bytes(),
Value: account1,
Proof: [][]byte{{248, 113, 160, 87, 118, 82, 182, 37, 183, 123, 219, 91, 247, 123, 196, 63, 49, 37, 202, 215, 70, 77, 103, 157, 21, 117, 86, 82, 119, 211, 97, 27, 128, 83, 231, 128, 128, 128, 128, 160, 254, 136, 159, 16, 229, 219, 143, 44, 43, 243, 85, 146, 129, 82, 161, 127, 110, 59, 185, 154, 146, 65, 172, 109, 132, 199, 126, 98, 100, 80, 156, 121, 128, 128, 128, 128, 128, 128, 128, 128, 160, 17, 219, 12, 218, 52, 168, 150, 218, 190, 182, 131, 155, 176, 106, 56, 244, 149, 20, 207, 164, 134, 67, 89, 132, 235, 1, 59, 125, 249, 238, 133, 197, 128, 128},
{248, 107, 160, 57, 38, 219, 105, 170, 206, 213, 24, 233, 185, 240, 244, 52, 164, 115, 231, 23, 65, 9, 201, 67, 84, 139, 184, 242, 59, 228, 28, 167, 109, 154, 210, 184, 72, 248, 70, 128, 130, 39, 16, 160, 86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33, 160, 197, 210, 70, 1, 134, 247, 35, 60, 146, 126, 125, 178, 220, 199, 3, 192, 229, 0, 182, 83, 202, 130, 39, 59, 123, 250, 216, 4, 93, 133, 164, 112}},
Path: []byte{14, 9, 2, 6, 13, 11, 6, 9, 10, 10, 12, 14, 13, 5, 1, 8, 14, 9, 11, 9, 15, 0, 15, 4, 3, 4, 10, 4, 7, 3, 14, 7, 1, 7, 4, 1, 0, 9, 12, 9, 4, 3, 5, 4, 8, 11, 11, 8, 15, 2, 3, 11, 14, 4, 1, 12, 10, 7, 6, 13, 9, 10, 13, 2, 16},
Storage: []statediff.StorageDiff{},
},
burnLeafKey: {
CreatedAccounts: []statediff.AccountDiff{
{
Key: burnLeafKey.Bytes(),
Value: burnAccount1,
Proof: [][]byte{{248, 113, 160, 87, 118, 82, 182, 37, 183, 123, 219, 91, 247, 123, 196, 63, 49, 37, 202, 215, 70, 77, 103, 157, 21, 117, 86, 82, 119, 211, 97, 27, 128, 83, 231, 128, 128, 128, 128, 160, 254, 136, 159, 16, 229, 219, 143, 44, 43, 243, 85, 146, 129, 82, 161, 127, 110, 59, 185, 154, 146, 65, 172, 109, 132, 199, 126, 98, 100, 80, 156, 121, 128, 128, 128, 128, 128, 128, 128, 128, 160, 17, 219, 12, 218, 52, 168, 150, 218, 190, 182, 131, 155, 176, 106, 56, 244, 149, 20, 207, 164, 134, 67, 89, 132, 235, 1, 59, 125, 249, 238, 133, 197, 128, 128},
@@ -206,10 +198,18 @@ func TestBuilder(t *testing.T) {
Path: []byte{5, 3, 8, 0, 12, 7, 11, 7, 10, 14, 8, 1, 10, 5, 8, 14, 11, 9, 8, 13, 9, 12, 7, 8, 13, 14, 4, 10, 1, 15, 13, 7, 15, 13, 9, 5, 3, 5, 15, 12, 9, 5, 3, 14, 13, 2, 11, 14, 6, 0, 2, 13, 10, 10, 10, 4, 1, 7, 6, 7, 3, 1, 2, 10, 16},
Storage: []statediff.StorageDiff{},
},
{
Key: testhelpers.Account1LeafKey.Bytes(),
Value: account1,
Proof: [][]byte{{248, 113, 160, 87, 118, 82, 182, 37, 183, 123, 219, 91, 247, 123, 196, 63, 49, 37, 202, 215, 70, 77, 103, 157, 21, 117, 86, 82, 119, 211, 97, 27, 128, 83, 231, 128, 128, 128, 128, 160, 254, 136, 159, 16, 229, 219, 143, 44, 43, 243, 85, 146, 129, 82, 161, 127, 110, 59, 185, 154, 146, 65, 172, 109, 132, 199, 126, 98, 100, 80, 156, 121, 128, 128, 128, 128, 128, 128, 128, 128, 160, 17, 219, 12, 218, 52, 168, 150, 218, 190, 182, 131, 155, 176, 106, 56, 244, 149, 20, 207, 164, 134, 67, 89, 132, 235, 1, 59, 125, 249, 238, 133, 197, 128, 128},
{248, 107, 160, 57, 38, 219, 105, 170, 206, 213, 24, 233, 185, 240, 244, 52, 164, 115, 231, 23, 65, 9, 201, 67, 84, 139, 184, 242, 59, 228, 28, 167, 109, 154, 210, 184, 72, 248, 70, 128, 130, 39, 16, 160, 86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33, 160, 197, 210, 70, 1, 134, 247, 35, 60, 146, 126, 125, 178, 220, 199, 3, 192, 229, 0, 182, 83, 202, 130, 39, 59, 123, 250, 216, 4, 93, 133, 164, 112}},
Path: []byte{14, 9, 2, 6, 13, 11, 6, 9, 10, 10, 12, 14, 13, 5, 1, 8, 14, 9, 11, 9, 15, 0, 15, 4, 3, 4, 10, 4, 7, 3, 14, 7, 1, 7, 4, 1, 0, 9, 12, 9, 4, 3, 5, 4, 8, 11, 11, 8, 15, 2, 3, 11, 14, 4, 1, 12, 10, 7, 6, 13, 9, 10, 13, 2, 16},
Storage: []statediff.StorageDiff{},
},
},
DeletedAccounts: emptyAccountDiffEventualMap,
UpdatedAccounts: statediff.AccountDiffsMap{
testhelpers.BankLeafKey: {
UpdatedAccounts: []statediff.AccountDiff{
{
Key: testhelpers.BankLeafKey.Bytes(),
Value: bankAccount1,
Proof: [][]byte{{248, 113, 160, 87, 118, 82, 182, 37, 183, 123, 219, 91, 247, 123, 196, 63, 49, 37, 202, 215, 70, 77, 103, 157, 21, 117, 86, 82, 119, 211, 97, 27, 128, 83, 231, 128, 128, 128, 128, 160, 254, 136, 159, 16, 229, 219, 143, 44, 43, 243, 85, 146, 129, 82, 161, 127, 110, 59, 185, 154, 146, 65, 172, 109, 132, 199, 126, 98, 100, 80, 156, 121, 128, 128, 128, 128, 128, 128, 128, 128, 160, 17, 219, 12, 218, 52, 168, 150, 218, 190, 182, 131, 155, 176, 106, 56, 244, 149, 20, 207, 164, 134, 67, 89, 132, 235, 1, 59, 125, 249, 238, 133, 197, 128, 128},
@@ -227,22 +227,14 @@ func TestBuilder(t *testing.T) {
arguments{
oldStateRoot: block1.Root(),
newStateRoot: block2.Root(),
blockNumber: block2.Number().Int64(),
blockNumber: block2.Number(),
blockHash: block2Hash,
},
&statediff.StateDiff{
BlockNumber: block2.Number().Int64(),
BlockNumber: block2.Number(),
BlockHash: block2.Hash(),
CreatedAccounts: statediff.AccountDiffsMap{
testhelpers.Account2LeafKey: {
Key: testhelpers.Account2LeafKey.Bytes(),
Value: account2,
Proof: [][]byte{{248, 177, 160, 177, 155, 238, 178, 242, 47, 83, 2, 49, 141, 155, 92, 149, 175, 245, 120, 233, 177, 101, 67, 46, 200, 23, 250, 41, 74, 135, 94, 61, 133, 51, 162, 128, 128, 128, 128, 160, 179, 86, 53, 29, 96, 188, 152, 148, 207, 31, 29, 108, 182, 140, 129, 95, 1, 49, 213, 15, 29, 168, 60, 64, 35, 160, 158, 200, 85, 207, 255, 145, 160, 114, 57, 32, 11, 115, 232, 140, 238, 165, 222, 121, 226, 208, 2, 192, 216, 67, 198, 179, 31, 181, 27, 208, 243, 99, 202, 48, 148, 207, 107, 106, 177, 128, 128, 128, 128, 128, 160, 10, 173, 165, 125, 110, 240, 77, 112, 149, 100, 135, 237, 25, 228, 116, 7, 195, 9, 210, 166, 208, 148, 101, 23, 244, 238, 84, 84, 211, 249, 138, 137, 128, 160, 255, 115, 147, 190, 57, 135, 174, 188, 86, 51, 227, 70, 22, 253, 237, 49, 24, 19, 149, 199, 142, 195, 186, 244, 70, 51, 138, 0, 146, 148, 117, 60, 128, 128},
{248, 107, 160, 57, 87, 243, 226, 240, 74, 7, 100, 195, 160, 73, 27, 23, 95, 105, 146, 109, 166, 30, 251, 204, 143, 97, 250, 20, 85, 253, 45, 43, 76, 221, 69, 184, 72, 248, 70, 128, 130, 3, 232, 160, 86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33, 160, 197, 210, 70, 1, 134, 247, 35, 60, 146, 126, 125, 178, 220, 199, 3, 192, 229, 0, 182, 83, 202, 130, 39, 59, 123, 250, 216, 4, 93, 133, 164, 112}},
Path: []byte{12, 9, 5, 7, 15, 3, 14, 2, 15, 0, 4, 10, 0, 7, 6, 4, 12, 3, 10, 0, 4, 9, 1, 11, 1, 7, 5, 15, 6, 9, 9, 2, 6, 13, 10, 6, 1, 14, 15, 11, 12, 12, 8, 15, 6, 1, 15, 10, 1, 4, 5, 5, 15, 13, 2, 13, 2, 11, 4, 12, 13, 13, 4, 5, 16},
Storage: []statediff.StorageDiff{},
},
contractLeafKey: {
CreatedAccounts: []statediff.AccountDiff{
{
Key: contractLeafKey.Bytes(),
Value: contractAccount,
Proof: [][]byte{{248, 177, 160, 177, 155, 238, 178, 242, 47, 83, 2, 49, 141, 155, 92, 149, 175, 245, 120, 233, 177, 101, 67, 46, 200, 23, 250, 41, 74, 135, 94, 61, 133, 51, 162, 128, 128, 128, 128, 160, 179, 86, 53, 29, 96, 188, 152, 148, 207, 31, 29, 108, 182, 140, 129, 95, 1, 49, 213, 15, 29, 168, 60, 64, 35, 160, 158, 200, 85, 207, 255, 145, 160, 114, 57, 32, 11, 115, 232, 140, 238, 165, 222, 121, 226, 208, 2, 192, 216, 67, 198, 179, 31, 181, 27, 208, 243, 99, 202, 48, 148, 207, 107, 106, 177, 128, 128, 128, 128, 128, 160, 10, 173, 165, 125, 110, 240, 77, 112, 149, 100, 135, 237, 25, 228, 116, 7, 195, 9, 210, 166, 208, 148, 101, 23, 244, 238, 84, 84, 211, 249, 138, 137, 128, 160, 255, 115, 147, 190, 57, 135, 174, 188, 86, 51, 227, 70, 22, 253, 237, 49, 24, 19, 149, 199, 142, 195, 186, 244, 70, 51, 138, 0, 146, 148, 117, 60, 128, 128},
@@ -257,10 +249,18 @@ func TestBuilder(t *testing.T) {
},
},
},
{
Key: testhelpers.Account2LeafKey.Bytes(),
Value: account2,
Proof: [][]byte{{248, 177, 160, 177, 155, 238, 178, 242, 47, 83, 2, 49, 141, 155, 92, 149, 175, 245, 120, 233, 177, 101, 67, 46, 200, 23, 250, 41, 74, 135, 94, 61, 133, 51, 162, 128, 128, 128, 128, 160, 179, 86, 53, 29, 96, 188, 152, 148, 207, 31, 29, 108, 182, 140, 129, 95, 1, 49, 213, 15, 29, 168, 60, 64, 35, 160, 158, 200, 85, 207, 255, 145, 160, 114, 57, 32, 11, 115, 232, 140, 238, 165, 222, 121, 226, 208, 2, 192, 216, 67, 198, 179, 31, 181, 27, 208, 243, 99, 202, 48, 148, 207, 107, 106, 177, 128, 128, 128, 128, 128, 160, 10, 173, 165, 125, 110, 240, 77, 112, 149, 100, 135, 237, 25, 228, 116, 7, 195, 9, 210, 166, 208, 148, 101, 23, 244, 238, 84, 84, 211, 249, 138, 137, 128, 160, 255, 115, 147, 190, 57, 135, 174, 188, 86, 51, 227, 70, 22, 253, 237, 49, 24, 19, 149, 199, 142, 195, 186, 244, 70, 51, 138, 0, 146, 148, 117, 60, 128, 128},
{248, 107, 160, 57, 87, 243, 226, 240, 74, 7, 100, 195, 160, 73, 27, 23, 95, 105, 146, 109, 166, 30, 251, 204, 143, 97, 250, 20, 85, 253, 45, 43, 76, 221, 69, 184, 72, 248, 70, 128, 130, 3, 232, 160, 86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33, 160, 197, 210, 70, 1, 134, 247, 35, 60, 146, 126, 125, 178, 220, 199, 3, 192, 229, 0, 182, 83, 202, 130, 39, 59, 123, 250, 216, 4, 93, 133, 164, 112}},
Path: []byte{12, 9, 5, 7, 15, 3, 14, 2, 15, 0, 4, 10, 0, 7, 6, 4, 12, 3, 10, 0, 4, 9, 1, 11, 1, 7, 5, 15, 6, 9, 9, 2, 6, 13, 10, 6, 1, 14, 15, 11, 12, 12, 8, 15, 6, 1, 15, 10, 1, 4, 5, 5, 15, 13, 2, 13, 2, 11, 4, 12, 13, 13, 4, 5, 16},
Storage: []statediff.StorageDiff{},
},
},
DeletedAccounts: emptyAccountDiffEventualMap,
UpdatedAccounts: statediff.AccountDiffsMap{
testhelpers.BankLeafKey: {
UpdatedAccounts: []statediff.AccountDiff{
{
Key: testhelpers.BankLeafKey.Bytes(),
Value: bankAccount2,
Proof: [][]byte{{248, 177, 160, 177, 155, 238, 178, 242, 47, 83, 2, 49, 141, 155, 92, 149, 175, 245, 120, 233, 177, 101, 67, 46, 200, 23, 250, 41, 74, 135, 94, 61, 133, 51, 162, 128, 128, 128, 128, 160, 179, 86, 53, 29, 96, 188, 152, 148, 207, 31, 29, 108, 182, 140, 129, 95, 1, 49, 213, 15, 29, 168, 60, 64, 35, 160, 158, 200, 85, 207, 255, 145, 160, 114, 57, 32, 11, 115, 232, 140, 238, 165, 222, 121, 226, 208, 2, 192, 216, 67, 198, 179, 31, 181, 27, 208, 243, 99, 202, 48, 148, 207, 107, 106, 177, 128, 128, 128, 128, 128, 160, 10, 173, 165, 125, 110, 240, 77, 112, 149, 100, 135, 237, 25, 228, 116, 7, 195, 9, 210, 166, 208, 148, 101, 23, 244, 238, 84, 84, 211, 249, 138, 137, 128, 160, 255, 115, 147, 190, 57, 135, 174, 188, 86, 51, 227, 70, 22, 253, 237, 49, 24, 19, 149, 199, 142, 195, 186, 244, 70, 51, 138, 0, 146, 148, 117, 60, 128, 128},
@@ -268,15 +268,7 @@ func TestBuilder(t *testing.T) {
Path: []byte{0, 0, 11, 15, 4, 9, 15, 4, 4, 0, 10, 1, 12, 13, 0, 5, 2, 7, 14, 4, 13, 0, 6, 14, 2, 7, 6, 5, 6, 5, 4, 12, 0, 15, 5, 6, 4, 5, 2, 2, 5, 7, 5, 1, 6, 13, 7, 9, 3, 10, 9, 11, 8, 13, 6, 0, 4, 13, 12, 15, 13, 15, 2, 10, 16},
Storage: []statediff.StorageDiff{},
},
testhelpers.Account1LeafKey: {
Key: testhelpers.Account1LeafKey.Bytes(),
Value: account3,
Proof: [][]byte{{248, 177, 160, 177, 155, 238, 178, 242, 47, 83, 2, 49, 141, 155, 92, 149, 175, 245, 120, 233, 177, 101, 67, 46, 200, 23, 250, 41, 74, 135, 94, 61, 133, 51, 162, 128, 128, 128, 128, 160, 179, 86, 53, 29, 96, 188, 152, 148, 207, 31, 29, 108, 182, 140, 129, 95, 1, 49, 213, 15, 29, 168, 60, 64, 35, 160, 158, 200, 85, 207, 255, 145, 160, 114, 57, 32, 11, 115, 232, 140, 238, 165, 222, 121, 226, 208, 2, 192, 216, 67, 198, 179, 31, 181, 27, 208, 243, 99, 202, 48, 148, 207, 107, 106, 177, 128, 128, 128, 128, 128, 160, 10, 173, 165, 125, 110, 240, 77, 112, 149, 100, 135, 237, 25, 228, 116, 7, 195, 9, 210, 166, 208, 148, 101, 23, 244, 238, 84, 84, 211, 249, 138, 137, 128, 160, 255, 115, 147, 190, 57, 135, 174, 188, 86, 51, 227, 70, 22, 253, 237, 49, 24, 19, 149, 199, 142, 195, 186, 244, 70, 51, 138, 0, 146, 148, 117, 60, 128, 128},
{248, 107, 160, 57, 38, 219, 105, 170, 206, 213, 24, 233, 185, 240, 244, 52, 164, 115, 231, 23, 65, 9, 201, 67, 84, 139, 184, 242, 59, 228, 28, 167, 109, 154, 210, 184, 72, 248, 70, 2, 130, 39, 16, 160, 86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33, 160, 197, 210, 70, 1, 134, 247, 35, 60, 146, 126, 125, 178, 220, 199, 3, 192, 229, 0, 182, 83, 202, 130, 39, 59, 123, 250, 216, 4, 93, 133, 164, 112}},
Path: []byte{14, 9, 2, 6, 13, 11, 6, 9, 10, 10, 12, 14, 13, 5, 1, 8, 14, 9, 11, 9, 15, 0, 15, 4, 3, 4, 10, 4, 7, 3, 14, 7, 1, 7, 4, 1, 0, 9, 12, 9, 4, 3, 5, 4, 8, 11, 11, 8, 15, 2, 3, 11, 14, 4, 1, 12, 10, 7, 6, 13, 9, 10, 13, 2, 16},
Storage: []statediff.StorageDiff{},
},
burnLeafKey: {
{
Key: burnLeafKey.Bytes(),
Value: burnAccount2,
Proof: [][]byte{{248, 177, 160, 177, 155, 238, 178, 242, 47, 83, 2, 49, 141, 155, 92, 149, 175, 245, 120, 233, 177, 101, 67, 46, 200, 23, 250, 41, 74, 135, 94, 61, 133, 51, 162, 128, 128, 128, 128, 160, 179, 86, 53, 29, 96, 188, 152, 148, 207, 31, 29, 108, 182, 140, 129, 95, 1, 49, 213, 15, 29, 168, 60, 64, 35, 160, 158, 200, 85, 207, 255, 145, 160, 114, 57, 32, 11, 115, 232, 140, 238, 165, 222, 121, 226, 208, 2, 192, 216, 67, 198, 179, 31, 181, 27, 208, 243, 99, 202, 48, 148, 207, 107, 106, 177, 128, 128, 128, 128, 128, 160, 10, 173, 165, 125, 110, 240, 77, 112, 149, 100, 135, 237, 25, 228, 116, 7, 195, 9, 210, 166, 208, 148, 101, 23, 244, 238, 84, 84, 211, 249, 138, 137, 128, 160, 255, 115, 147, 190, 57, 135, 174, 188, 86, 51, 227, 70, 22, 253, 237, 49, 24, 19, 149, 199, 142, 195, 186, 244, 70, 51, 138, 0, 146, 148, 117, 60, 128, 128},
@@ -284,6 +276,14 @@ func TestBuilder(t *testing.T) {
Path: []byte{5, 3, 8, 0, 12, 7, 11, 7, 10, 14, 8, 1, 10, 5, 8, 14, 11, 9, 8, 13, 9, 12, 7, 8, 13, 14, 4, 10, 1, 15, 13, 7, 15, 13, 9, 5, 3, 5, 15, 12, 9, 5, 3, 14, 13, 2, 11, 14, 6, 0, 2, 13, 10, 10, 10, 4, 1, 7, 6, 7, 3, 1, 2, 10, 16},
Storage: []statediff.StorageDiff{},
},
{
Key: testhelpers.Account1LeafKey.Bytes(),
Value: account3,
Proof: [][]byte{{248, 177, 160, 177, 155, 238, 178, 242, 47, 83, 2, 49, 141, 155, 92, 149, 175, 245, 120, 233, 177, 101, 67, 46, 200, 23, 250, 41, 74, 135, 94, 61, 133, 51, 162, 128, 128, 128, 128, 160, 179, 86, 53, 29, 96, 188, 152, 148, 207, 31, 29, 108, 182, 140, 129, 95, 1, 49, 213, 15, 29, 168, 60, 64, 35, 160, 158, 200, 85, 207, 255, 145, 160, 114, 57, 32, 11, 115, 232, 140, 238, 165, 222, 121, 226, 208, 2, 192, 216, 67, 198, 179, 31, 181, 27, 208, 243, 99, 202, 48, 148, 207, 107, 106, 177, 128, 128, 128, 128, 128, 160, 10, 173, 165, 125, 110, 240, 77, 112, 149, 100, 135, 237, 25, 228, 116, 7, 195, 9, 210, 166, 208, 148, 101, 23, 244, 238, 84, 84, 211, 249, 138, 137, 128, 160, 255, 115, 147, 190, 57, 135, 174, 188, 86, 51, 227, 70, 22, 253, 237, 49, 24, 19, 149, 199, 142, 195, 186, 244, 70, 51, 138, 0, 146, 148, 117, 60, 128, 128},
{248, 107, 160, 57, 38, 219, 105, 170, 206, 213, 24, 233, 185, 240, 244, 52, 164, 115, 231, 23, 65, 9, 201, 67, 84, 139, 184, 242, 59, 228, 28, 167, 109, 154, 210, 184, 72, 248, 70, 2, 130, 39, 16, 160, 86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33, 160, 197, 210, 70, 1, 134, 247, 35, 60, 146, 126, 125, 178, 220, 199, 3, 192, 229, 0, 182, 83, 202, 130, 39, 59, 123, 250, 216, 4, 93, 133, 164, 112}},
Path: []byte{14, 9, 2, 6, 13, 11, 6, 9, 10, 10, 12, 14, 13, 5, 1, 8, 14, 9, 11, 9, 15, 0, 15, 4, 3, 4, 10, 4, 7, 3, 14, 7, 1, 7, 4, 1, 0, 9, 12, 9, 4, 3, 5, 4, 8, 11, 11, 8, 15, 2, 3, 11, 14, 4, 1, 12, 10, 7, 6, 13, 9, 10, 13, 2, 16},
Storage: []statediff.StorageDiff{},
},
},
},
},
@@ -294,24 +294,24 @@ func TestBuilder(t *testing.T) {
arguments{
oldStateRoot: block2.Root(),
newStateRoot: block3.Root(),
blockNumber: block3.Number().Int64(),
blockNumber: block3.Number(),
blockHash: block3.Hash(),
},
&statediff.StateDiff{
BlockNumber: block3.Number().Int64(),
BlockNumber: block3.Number(),
BlockHash: block3.Hash(),
CreatedAccounts: statediff.AccountDiffsMap{},
CreatedAccounts: []statediff.AccountDiff{},
DeletedAccounts: emptyAccountDiffEventualMap,
UpdatedAccounts: statediff.AccountDiffsMap{
testhelpers.Account2LeafKey: {
Key: testhelpers.Account2LeafKey.Bytes(),
Value: account4,
UpdatedAccounts: []statediff.AccountDiff{
{
Key: testhelpers.BankLeafKey.Bytes(),
Value: bankAccount3,
Proof: [][]byte{{248, 177, 160, 101, 223, 138, 81, 34, 40, 229, 170, 198, 188, 136, 99, 7, 55, 33, 112, 160, 111, 181, 131, 167, 201, 131, 24, 201, 211, 177, 30, 159, 229, 246, 6, 128, 128, 128, 128, 160, 179, 86, 53, 29, 96, 188, 152, 148, 207, 31, 29, 108, 182, 140, 129, 95, 1, 49, 213, 15, 29, 168, 60, 64, 35, 160, 158, 200, 85, 207, 255, 145, 160, 32, 135, 108, 213, 150, 150, 110, 44, 170, 65, 75, 154, 74, 249, 94, 65, 74, 107, 100, 115, 39, 5, 3, 26, 22, 238, 138, 114, 254, 21, 6, 171, 128, 128, 128, 128, 128, 160, 4, 228, 121, 222, 255, 218, 60, 247, 15, 0, 34, 198, 28, 229, 180, 129, 109, 157, 68, 181, 248, 229, 200, 123, 29, 81, 145, 114, 90, 209, 205, 210, 128, 160, 255, 115, 147, 190, 57, 135, 174, 188, 86, 51, 227, 70, 22, 253, 237, 49, 24, 19, 149, 199, 142, 195, 186, 244, 70, 51, 138, 0, 146, 148, 117, 60, 128, 128},
{248, 113, 160, 57, 87, 243, 226, 240, 74, 7, 100, 195, 160, 73, 27, 23, 95, 105, 146, 109, 166, 30, 251, 204, 143, 97, 250, 20, 85, 253, 45, 43, 76, 221, 69, 184, 78, 248, 76, 128, 136, 27, 193, 109, 103, 78, 200, 3, 232, 160, 86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33, 160, 197, 210, 70, 1, 134, 247, 35, 60, 146, 126, 125, 178, 220, 199, 3, 192, 229, 0, 182, 83, 202, 130, 39, 59, 123, 250, 216, 4, 93, 133, 164, 112}},
Path: []byte{12, 9, 5, 7, 15, 3, 14, 2, 15, 0, 4, 10, 0, 7, 6, 4, 12, 3, 10, 0, 4, 9, 1, 11, 1, 7, 5, 15, 6, 9, 9, 2, 6, 13, 10, 6, 1, 14, 15, 11, 12, 12, 8, 15, 6, 1, 15, 10, 1, 4, 5, 5, 15, 13, 2, 13, 2, 11, 4, 12, 13, 13, 4, 5, 16},
{248, 109, 160, 48, 191, 73, 244, 64, 161, 205, 5, 39, 228, 208, 110, 39, 101, 101, 76, 15, 86, 69, 34, 87, 81, 109, 121, 58, 155, 141, 96, 77, 207, 223, 42, 184, 74, 248, 72, 3, 132, 5, 245, 182, 8, 160, 86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33, 160, 197, 210, 70, 1, 134, 247, 35, 60, 146, 126, 125, 178, 220, 199, 3, 192, 229, 0, 182, 83, 202, 130, 39, 59, 123, 250, 216, 4, 93, 133, 164, 112}},
Path: []byte{0, 0, 11, 15, 4, 9, 15, 4, 4, 0, 10, 1, 12, 13, 0, 5, 2, 7, 14, 4, 13, 0, 6, 14, 2, 7, 6, 5, 6, 5, 4, 12, 0, 15, 5, 6, 4, 5, 2, 2, 5, 7, 5, 1, 6, 13, 7, 9, 3, 10, 9, 11, 8, 13, 6, 0, 4, 13, 12, 15, 13, 15, 2, 10, 16},
Storage: []statediff.StorageDiff{},
},
contractLeafKey: {
{
Key: contractLeafKey.Bytes(),
Value: contractAccount2,
Proof: [][]byte{{248, 177, 160, 101, 223, 138, 81, 34, 40, 229, 170, 198, 188, 136, 99, 7, 55, 33, 112, 160, 111, 181, 131, 167, 201, 131, 24, 201, 211, 177, 30, 159, 229, 246, 6, 128, 128, 128, 128, 160, 179, 86, 53, 29, 96, 188, 152, 148, 207, 31, 29, 108, 182, 140, 129, 95, 1, 49, 213, 15, 29, 168, 60, 64, 35, 160, 158, 200, 85, 207, 255, 145, 160, 32, 135, 108, 213, 150, 150, 110, 44, 170, 65, 75, 154, 74, 249, 94, 65, 74, 107, 100, 115, 39, 5, 3, 26, 22, 238, 138, 114, 254, 21, 6, 171, 128, 128, 128, 128, 128, 160, 4, 228, 121, 222, 255, 218, 60, 247, 15, 0, 34, 198, 28, 229, 180, 129, 109, 157, 68, 181, 248, 229, 200, 123, 29, 81, 145, 114, 90, 209, 205, 210, 128, 160, 255, 115, 147, 190, 57, 135, 174, 188, 86, 51, 227, 70, 22, 253, 237, 49, 24, 19, 149, 199, 142, 195, 186, 244, 70, 51, 138, 0, 146, 148, 117, 60, 128, 128},
@@ -327,12 +327,12 @@ func TestBuilder(t *testing.T) {
},
},
},
testhelpers.BankLeafKey: {
Key: testhelpers.BankLeafKey.Bytes(),
Value: bankAccount3,
{
Key: testhelpers.Account2LeafKey.Bytes(),
Value: account4,
Proof: [][]byte{{248, 177, 160, 101, 223, 138, 81, 34, 40, 229, 170, 198, 188, 136, 99, 7, 55, 33, 112, 160, 111, 181, 131, 167, 201, 131, 24, 201, 211, 177, 30, 159, 229, 246, 6, 128, 128, 128, 128, 160, 179, 86, 53, 29, 96, 188, 152, 148, 207, 31, 29, 108, 182, 140, 129, 95, 1, 49, 213, 15, 29, 168, 60, 64, 35, 160, 158, 200, 85, 207, 255, 145, 160, 32, 135, 108, 213, 150, 150, 110, 44, 170, 65, 75, 154, 74, 249, 94, 65, 74, 107, 100, 115, 39, 5, 3, 26, 22, 238, 138, 114, 254, 21, 6, 171, 128, 128, 128, 128, 128, 160, 4, 228, 121, 222, 255, 218, 60, 247, 15, 0, 34, 198, 28, 229, 180, 129, 109, 157, 68, 181, 248, 229, 200, 123, 29, 81, 145, 114, 90, 209, 205, 210, 128, 160, 255, 115, 147, 190, 57, 135, 174, 188, 86, 51, 227, 70, 22, 253, 237, 49, 24, 19, 149, 199, 142, 195, 186, 244, 70, 51, 138, 0, 146, 148, 117, 60, 128, 128},
{248, 109, 160, 48, 191, 73, 244, 64, 161, 205, 5, 39, 228, 208, 110, 39, 101, 101, 76, 15, 86, 69, 34, 87, 81, 109, 121, 58, 155, 141, 96, 77, 207, 223, 42, 184, 74, 248, 72, 3, 132, 5, 245, 182, 8, 160, 86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33, 160, 197, 210, 70, 1, 134, 247, 35, 60, 146, 126, 125, 178, 220, 199, 3, 192, 229, 0, 182, 83, 202, 130, 39, 59, 123, 250, 216, 4, 93, 133, 164, 112}},
Path: []byte{0, 0, 11, 15, 4, 9, 15, 4, 4, 0, 10, 1, 12, 13, 0, 5, 2, 7, 14, 4, 13, 0, 6, 14, 2, 7, 6, 5, 6, 5, 4, 12, 0, 15, 5, 6, 4, 5, 2, 2, 5, 7, 5, 1, 6, 13, 7, 9, 3, 10, 9, 11, 8, 13, 6, 0, 4, 13, 12, 15, 13, 15, 2, 10, 16},
{248, 113, 160, 57, 87, 243, 226, 240, 74, 7, 100, 195, 160, 73, 27, 23, 95, 105, 146, 109, 166, 30, 251, 204, 143, 97, 250, 20, 85, 253, 45, 43, 76, 221, 69, 184, 78, 248, 76, 128, 136, 27, 193, 109, 103, 78, 200, 3, 232, 160, 86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33, 160, 197, 210, 70, 1, 134, 247, 35, 60, 146, 126, 125, 178, 220, 199, 3, 192, 229, 0, 182, 83, 202, 130, 39, 59, 123, 250, 216, 4, 93, 133, 164, 112}},
Path: []byte{12, 9, 5, 7, 15, 3, 14, 2, 15, 0, 4, 10, 0, 7, 6, 4, 12, 3, 10, 0, 4, 9, 1, 11, 1, 7, 5, 15, 6, 9, 9, 2, 6, 13, 10, 6, 1, 14, 15, 11, 12, 12, 8, 15, 6, 1, 15, 10, 1, 4, 5, 5, 15, 13, 2, 13, 2, 11, 4, 12, 13, 13, 4, 5, 16},
Storage: []statediff.StorageDiff{},
},
},
@@ -346,36 +346,23 @@ func TestBuilder(t *testing.T) {
if err != nil {
t.Error(err)
}
fields := []string{"BlockNumber", "BlockHash", "DeletedAccounts", "UpdatedAccounts", "CreatedAccounts"}
for _, field := range fields {
reflectionOfDiff := reflect.ValueOf(diff)
diffValue := reflect.Indirect(reflectionOfDiff).FieldByName(field)
reflectionOfExpected := reflect.ValueOf(test.expected)
expectedValue := reflect.Indirect(reflectionOfExpected).FieldByName(field)
diffValueInterface := diffValue.Interface()
expectedValueInterface := expectedValue.Interface()
if !equals(diffValueInterface, expectedValueInterface) {
t.Logf("Test failed: %s", test.name)
t.Errorf("field: %+v\nactual: %+v\nexpected: %+v", field, diffValueInterface, expectedValueInterface)
}
receivedStateDiffRlp, err := rlp.EncodeToBytes(diff)
if err != nil {
t.Error(err)
}
expectedStateDiffRlp, err := rlp.EncodeToBytes(test.expected)
if err != nil {
t.Error(err)
}
sort.Slice(receivedStateDiffRlp, func(i, j int) bool { return receivedStateDiffRlp[i] < receivedStateDiffRlp[j] })
sort.Slice(expectedStateDiffRlp, func(i, j int) bool { return expectedStateDiffRlp[i] < expectedStateDiffRlp[j] })
if !bytes.Equal(receivedStateDiffRlp, expectedStateDiffRlp) {
t.Logf("Test failed: %s", test.name)
t.Errorf("actual state diff rlp: %+v\nexpected state diff rlp: %+v", receivedStateDiffRlp, expectedStateDiffRlp)
}
}
}
func equals(actual, expected interface{}) (success bool) {
if actualByteSlice, ok := actual.([]byte); ok {
if expectedByteSlice, ok := expected.([]byte); ok {
return bytes.Equal(actualByteSlice, expectedByteSlice)
}
}
return reflect.DeepEqual(actual, expected)
}
/*
contract test {
+19 -7
View File
@@ -18,10 +18,11 @@ package statediff
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
import (
"bytes"
"encoding/json"
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
"fmt"
"sync"
"github.com/ethereum/go-ethereum/rlp"
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core"
"github.com/ethereum/go-ethereum/core/types"
2
@@ -63,6 +64,8 @@ type Service struct {
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
QuitChan chan bool
// A mapping of rpc.IDs to their subscription channels
Subscriptions map[rpc.ID]Subscription
// Cache the last block so that we can avoid having to lookup the next block's parent
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
lastBlock *types.Block
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
}
// NewStateDiffService creates a new StateDiffingService
@@ -106,7 +109,13 @@ func (sds *Service) Loop(chainEventCh chan core.ChainEvent) {
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
log.Debug("Event received from chainEventCh", "event", chainEvent)
currentBlock := chainEvent.Block
parentHash := currentBlock.ParentHash()
parentBlock := sds.BlockChain.GetBlockByHash(parentHash)
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
var parentBlock *types.Block
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
if sds.lastBlock != nil && bytes.Equal(sds.lastBlock.Hash().Bytes(), currentBlock.ParentHash().Bytes()) {
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
parentBlock = sds.lastBlock
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
} else {
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
parentBlock = sds.BlockChain.GetBlockByHash(parentHash)
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
}
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
sds.lastBlock = currentBlock
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
if parentBlock == nil {
log.Error("Parent block is nil, skipping this block",
"parent block hash", parentHash.String(),
2
@@ -130,7 +139,7 @@ func (sds *Service) Loop(chainEventCh chan core.ChainEvent) {
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
// process method builds the state diff payload from the current and parent block and streams it to listening subscriptions
func (sds *Service) process(currentBlock, parentBlock *types.Block) error {
stateDiff, err := sds.Builder.BuildStateDiff(parentBlock.Root(), currentBlock.Root(), currentBlock.Number().Int64(), currentBlock.Hash())
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
stateDiff, err := sds.Builder.BuildStateDiff(parentBlock.Root(), currentBlock.Root(), currentBlock.Number(), currentBlock.Hash())
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
if err != nil {
return err
}
@@ -138,11 +147,14 @@ func (sds *Service) process(currentBlock, parentBlock *types.Block) error {
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
rlpBuff := new(bytes.Buffer)
currentBlock.EncodeRLP(rlpBuff)
blockRlp := rlpBuff.Bytes()
stateDiffBytes, _ := json.Marshal(stateDiff)
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
stateDiffRlp, err := rlp.EncodeToBytes(stateDiff)
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
if err != nil {
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
return err
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
}
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
payload := Payload{
BlockRlp: blockRlp,
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
StateDiff: stateDiffBytes,
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
Err: err,
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
BlockRlp: blockRlp,
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
StateDiffRlp: stateDiffRlp,
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
Err: err,
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
}
// If we have any websocket subscription listening in, send the data to them
5
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
elizabethengelman commented 2019-07-22 15:31:19 +00:00 (Migrated from github.com)
Review

isn't this send method sending data to the rpc subscriptions as well?

isn't this `send` method sending data to the rpc subscriptions as well?
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
Review

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.

Yes, thank you! That is poor wording on my part. It sends to rpc subscriptions- whether or not those subscriptions are over websocket or ipc.
+4 -2
View File
@@ -17,6 +17,8 @@
package mocks
import (
"math/big"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/statediff"
)
@@ -25,14 +27,14 @@ import (
type Builder struct {
OldStateRoot common.Hash
NewStateRoot common.Hash
BlockNumber int64
BlockNumber *big.Int
BlockHash common.Hash
stateDiff statediff.StateDiff
builderError error
}
// BuildStateDiff mock method
func (builder *Builder) BuildStateDiff(oldStateRoot, newStateRoot common.Hash, blockNumber int64, blockHash common.Hash) (statediff.StateDiff, error) {
func (builder *Builder) BuildStateDiff(oldStateRoot, newStateRoot common.Hash, blockNumber *big.Int, blockHash common.Hash) (statediff.StateDiff, error) {
builder.OldStateRoot = oldStateRoot
builder.NewStateRoot = newStateRoot
builder.BlockNumber = blockNumber
+10 -6
View File
@@ -18,11 +18,12 @@ package mocks
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
import (
"bytes"
"encoding/json"
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
"errors"
"fmt"
"sync"
"github.com/ethereum/go-ethereum/rlp"
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
"github.com/ethereum/go-ethereum/core"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/log"
@@ -76,18 +77,21 @@ func (sds *MockStateDiffService) Loop(chan core.ChainEvent) {
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
continue
}
stateDiff, err := sds.Builder.BuildStateDiff(parentBlock.Root(), currentBlock.Root(), currentBlock.Number().Int64(), currentBlock.Hash())
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
stateDiff, err := sds.Builder.BuildStateDiff(parentBlock.Root(), currentBlock.Root(), currentBlock.Number(), currentBlock.Hash())
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
if err != nil {
log.Error("Error building statediff", "block number", currentBlock.Number(), "error", err)
}
rlpBuff := new(bytes.Buffer)
currentBlock.EncodeRLP(rlpBuff)
blockRlp := rlpBuff.Bytes()
stateDiffBytes, _ := json.Marshal(stateDiff)
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
stateDiffRlp, err := rlp.EncodeToBytes(stateDiff)
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
if err != nil {
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
log.Error("Error encoding statediff", "block number", currentBlock.Number(), "error", err)
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
}
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
payload := statediff.Payload{
BlockRlp: blockRlp,
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
StateDiff: stateDiffBytes,
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
Err: err,
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
BlockRlp: blockRlp,
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
StateDiffRlp: stateDiffRlp,
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
Err: err,
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
}
// If we have any websocket subscription listening in, send the data to them
sds.send(payload)
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
elizabethengelman commented 2019-05-21 17:58:29 +00:00 (Migrated from github.com)
Review

I may be missing something, but I only see this MockStateDiffService is statediff/testhelpers/mocks/service_test.go which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?

I may be missing something, but I only see this `MockStateDiffService` is `statediff/testhelpers/mocks/service_test.go` which seems to be testing the mock. Is the intention that this mock can also be used to test the API portion or something?
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
Review

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in statediff/testhelpers/mocks/service_test.go uses this MockStateDiffService which allows us to pass in current and parent blocks by its channels.

We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).

This mock is used to test the API and service together, the service_test that uses it has to be moved in here to prevent a disallowed import cycle (a disadvantage to the really flat package organization that geth uses). The service_test in the top level statediff package uses the real service which has been configured with all mocks, the service_test in `statediff/testhelpers/mocks/service_test.go` uses this `MockStateDiffService` which allows us to pass in current and parent blocks by its channels. We are reusing the same patterns as the rest of the services and apis in Geth so we shouldn't need to write tests for everything, so this test might be superfluous (when comparing the test we have to the test coverage in other service packages).
+22 -19
View File
@@ -18,8 +18,8 @@ package mocks
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
import (
"bytes"
"encoding/json"
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
"math/big"
"sort"
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
"sync"
"testing"
@@ -34,7 +34,7 @@ import (
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
var block0, block1 *types.Block
var burnLeafKey = testhelpers.AddressToLeafKey(common.HexToAddress("0x0"))
var emptyAccountDiffEventualMap = make(statediff.AccountDiffsMap)
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
var emptyAccountDiffEventualMap = make([]statediff.AccountDiff, 0)
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
var account1, _ = rlp.EncodeToBytes(state.Account{
Nonce: uint64(0),
Balance: big.NewInt(10000),
@@ -84,18 +84,10 @@ func TestAPI(t *testing.T) {
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
parentBlockChain <- block0
expectedBlockRlp, _ := rlp.EncodeToBytes(block1)
expectedStateDiff := statediff.StateDiff{
BlockNumber: block1.Number().Int64(),
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
BlockNumber: block1.Number(),
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
BlockHash: block1.Hash(),
CreatedAccounts: statediff.AccountDiffsMap{
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
testhelpers.Account1LeafKey: {
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Key: testhelpers.Account1LeafKey.Bytes(),
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Value: account1,
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Proof: [][]byte{{248, 113, 160, 87, 118, 82, 182, 37, 183, 123, 219, 91, 247, 123, 196, 63, 49, 37, 202, 215, 70, 77, 103, 157, 21, 117, 86, 82, 119, 211, 97, 27, 128, 83, 231, 128, 128, 128, 128, 160, 254, 136, 159, 16, 229, 219, 143, 44, 43, 243, 85, 146, 129, 82, 161, 127, 110, 59, 185, 154, 146, 65, 172, 109, 132, 199, 126, 98, 100, 80, 156, 121, 128, 128, 128, 128, 128, 128, 128, 128, 160, 17, 219, 12, 218, 52, 168, 150, 218, 190, 182, 131, 155, 176, 106, 56, 244, 149, 20, 207, 164, 134, 67, 89, 132, 235, 1, 59, 125, 249, 238, 133, 197, 128, 128},
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
{248, 107, 160, 57, 38, 219, 105, 170, 206, 213, 24, 233, 185, 240, 244, 52, 164, 115, 231, 23, 65, 9, 201, 67, 84, 139, 184, 242, 59, 228, 28, 167, 109, 154, 210, 184, 72, 248, 70, 128, 130, 39, 16, 160, 86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33, 160, 197, 210, 70, 1, 134, 247, 35, 60, 146, 126, 125, 178, 220, 199, 3, 192, 229, 0, 182, 83, 202, 130, 39, 59, 123, 250, 216, 4, 93, 133, 164, 112}},
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Path: []byte{14, 9, 2, 6, 13, 11, 6, 9, 10, 10, 12, 14, 13, 5, 1, 8, 14, 9, 11, 9, 15, 0, 15, 4, 3, 4, 10, 4, 7, 3, 14, 7, 1, 7, 4, 1, 0, 9, 12, 9, 4, 3, 5, 4, 8, 11, 11, 8, 15, 2, 3, 11, 14, 4, 1, 12, 10, 7, 6, 13, 9, 10, 13, 2, 16},
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Storage: []statediff.StorageDiff{},
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
},
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
burnLeafKey: {
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
CreatedAccounts: []statediff.AccountDiff{
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
{
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Key: burnLeafKey.Bytes(),
Value: burnAccount1,
Proof: [][]byte{{248, 113, 160, 87, 118, 82, 182, 37, 183, 123, 219, 91, 247, 123, 196, 63, 49, 37, 202, 215, 70, 77, 103, 157, 21, 117, 86, 82, 119, 211, 97, 27, 128, 83, 231, 128, 128, 128, 128, 160, 254, 136, 159, 16, 229, 219, 143, 44, 43, 243, 85, 146, 129, 82, 161, 127, 110, 59, 185, 154, 146, 65, 172, 109, 132, 199, 126, 98, 100, 80, 156, 121, 128, 128, 128, 128, 128, 128, 128, 128, 160, 17, 219, 12, 218, 52, 168, 150, 218, 190, 182, 131, 155, 176, 106, 56, 244, 149, 20, 207, 164, 134, 67, 89, 132, 235, 1, 59, 125, 249, 238, 133, 197, 128, 128},
@@ -103,10 +95,18 @@ func TestAPI(t *testing.T) {
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Path: []byte{5, 3, 8, 0, 12, 7, 11, 7, 10, 14, 8, 1, 10, 5, 8, 14, 11, 9, 8, 13, 9, 12, 7, 8, 13, 14, 4, 10, 1, 15, 13, 7, 15, 13, 9, 5, 3, 5, 15, 12, 9, 5, 3, 14, 13, 2, 11, 14, 6, 0, 2, 13, 10, 10, 10, 4, 1, 7, 6, 7, 3, 1, 2, 10, 16},
Storage: []statediff.StorageDiff{},
},
{
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Key: testhelpers.Account1LeafKey.Bytes(),
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Value: account1,
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Proof: [][]byte{{248, 113, 160, 87, 118, 82, 182, 37, 183, 123, 219, 91, 247, 123, 196, 63, 49, 37, 202, 215, 70, 77, 103, 157, 21, 117, 86, 82, 119, 211, 97, 27, 128, 83, 231, 128, 128, 128, 128, 160, 254, 136, 159, 16, 229, 219, 143, 44, 43, 243, 85, 146, 129, 82, 161, 127, 110, 59, 185, 154, 146, 65, 172, 109, 132, 199, 126, 98, 100, 80, 156, 121, 128, 128, 128, 128, 128, 128, 128, 128, 160, 17, 219, 12, 218, 52, 168, 150, 218, 190, 182, 131, 155, 176, 106, 56, 244, 149, 20, 207, 164, 134, 67, 89, 132, 235, 1, 59, 125, 249, 238, 133, 197, 128, 128},
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
{248, 107, 160, 57, 38, 219, 105, 170, 206, 213, 24, 233, 185, 240, 244, 52, 164, 115, 231, 23, 65, 9, 201, 67, 84, 139, 184, 242, 59, 228, 28, 167, 109, 154, 210, 184, 72, 248, 70, 128, 130, 39, 16, 160, 86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33, 160, 197, 210, 70, 1, 134, 247, 35, 60, 146, 126, 125, 178, 220, 199, 3, 192, 229, 0, 182, 83, 202, 130, 39, 59, 123, 250, 216, 4, 93, 133, 164, 112}},
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Path: []byte{14, 9, 2, 6, 13, 11, 6, 9, 10, 10, 12, 14, 13, 5, 1, 8, 14, 9, 11, 9, 15, 0, 15, 4, 3, 4, 10, 4, 7, 3, 14, 7, 1, 7, 4, 1, 0, 9, 12, 9, 4, 3, 5, 4, 8, 11, 11, 8, 15, 2, 3, 11, 14, 4, 1, 12, 10, 7, 6, 13, 9, 10, 13, 2, 16},
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Storage: []statediff.StorageDiff{},
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
},
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
},
DeletedAccounts: emptyAccountDiffEventualMap,
UpdatedAccounts: statediff.AccountDiffsMap{
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
testhelpers.BankLeafKey: {
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
UpdatedAccounts: []statediff.AccountDiff{
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
{
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Key: testhelpers.BankLeafKey.Bytes(),
Value: bankAccount1,
Proof: [][]byte{{248, 113, 160, 87, 118, 82, 182, 37, 183, 123, 219, 91, 247, 123, 196, 63, 49, 37, 202, 215, 70, 77, 103, 157, 21, 117, 86, 82, 119, 211, 97, 27, 128, 83, 231, 128, 128, 128, 128, 160, 254, 136, 159, 16, 229, 219, 143, 44, 43, 243, 85, 146, 129, 82, 161, 127, 110, 59, 185, 154, 146, 65, 172, 109, 132, 199, 126, 98, 100, 80, 156, 121, 128, 128, 128, 128, 128, 128, 128, 128, 160, 17, 219, 12, 218, 52, 168, 150, 218, 190, 182, 131, 155, 176, 106, 56, 244, 149, 20, 207, 164, 134, 67, 89, 132, 235, 1, 59, 125, 249, 238, 133, 197, 128, 128},
@@ -116,17 +116,20 @@ func TestAPI(t *testing.T) {
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
},
},
}
expectedStateDiffBytes, err := json.Marshal(expectedStateDiff)
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
expectedStateDiffBytes, err := rlp.EncodeToBytes(expectedStateDiff)
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
if err != nil {
t.Error(err)
}
sort.Slice(expectedStateDiffBytes, func(i, j int) bool { return expectedStateDiffBytes[i] < expectedStateDiffBytes[j] })
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
select {
case payload := <-payloadChan:
if !bytes.Equal(payload.BlockRlp, expectedBlockRlp) {
t.Errorf("payload does not have expected block\r\actual: %v\r\nexpected: %v", payload.BlockRlp, expectedBlockRlp)
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
t.Errorf("payload does not have expected block\r\actual block rlp: %v\r\nexpected block rlp: %v", payload.BlockRlp, expectedBlockRlp)
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
}
if !bytes.Equal(payload.StateDiff, expectedStateDiffBytes) {
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
t.Errorf("payload does not have expected state diff\r\actual: %v\r\nexpected: %v", payload.StateDiff, expectedStateDiffBytes)
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
sort.Slice(payload.StateDiffRlp, func(i, j int) bool { return payload.StateDiffRlp[i] < payload.StateDiffRlp[j] })
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
if !bytes.Equal(payload.StateDiffRlp, expectedStateDiffBytes) {
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
t.Errorf("payload does not have expected state diff\r\actual state diff rlp: %v\r\nexpected state diff rlp: %v", payload.StateDiffRlp, expectedStateDiffBytes)
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
}
if payload.Err != nil {
t.Errorf("payload should not contain an error, but does: %v", payload.Err)
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
elizabethengelman commented 2019-05-21 17:59:27 +00:00 (Migrated from github.com)
Review

is this file testing the Mock service? or the API? I wonder if the file should be renamed?

is this file testing the Mock service? or the API? I wonder if the file should be renamed?
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
Review

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the statediff package into the other service_test file.

Sorry, see above, this is confusing and sloppy and only organized like this because of a disallowed import cycle if this test is moved up to the `statediff` package into the other service_test file.
+6 -6
View File
@@ -36,7 +36,7 @@ func AddressToLeafKey(address common.Address) common.Hash {
// Test variables
var (
BlockNumber = rand.Int63()
BlockNumber = big.NewInt(rand.Int63())
BlockHash = "0xfa40fbe2d98d98b3363a778d52f2bcd29d6790b9b3f3cab2b167fd12d3550f73"
CodeHash = common.Hex2Bytes("0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470")
NewNonceValue = rand.Uint64()
@@ -63,26 +63,26 @@ var (
CodeHash: CodeHash,
}
valueBytes, _ = rlp.EncodeToBytes(testAccount)
CreatedAccountDiffs = statediff.AccountDiffsMap{
ContractLeafKey: {
CreatedAccountDiffs = []statediff.AccountDiff{
{
Key: ContractLeafKey.Bytes(),
Value: valueBytes,
Storage: storage,
},
AnotherContractLeafKey: {
{
Key: AnotherContractLeafKey.Bytes(),
Value: valueBytes,
Storage: emptyStorage,
},
}
UpdatedAccountDiffs = statediff.AccountDiffsMap{ContractLeafKey: {
UpdatedAccountDiffs = []statediff.AccountDiff{{
Key: ContractLeafKey.Bytes(),
Value: valueBytes,
Storage: storage,
}}
DeletedAccountDiffs = statediff.AccountDiffsMap{ContractLeafKey: {
DeletedAccountDiffs = []statediff.AccountDiff{{
Key: ContractLeafKey.Bytes(),
Value: valueBytes,
Storage: storage,
+12 -38
View File
@@ -21,6 +21,7 @@ package statediff
import (
"encoding/json"
"math/big"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/state"
@@ -34,9 +35,9 @@ type Subscription struct {
// Payload packages the data to send to StateDiffingService subscriptions
type Payload struct {
BlockRlp []byte `json:"blockRlp" gencodec:"required"`
StateDiff []byte `json:"stateDiff" gencodec:"required"`
Err error `json:"error"`
BlockRlp []byte `json:"blockRlp" gencodec:"required"`
StateDiffRlp []byte `json:"stateDiff" gencodec:"required"`
Err error `json:"error"`
}
// AccountsMap is a mapping of keccak256(address) => accountWrapper
1
@@ -53,11 +54,11 @@ type accountWrapper struct {
// StateDiff is the final output structure from the builder
type StateDiff struct {
BlockNumber int64 `json:"blockNumber" gencodec:"required"`
BlockHash common.Hash `json:"blockHash" gencodec:"required"`
CreatedAccounts AccountDiffsMap `json:"createdAccounts" gencodec:"required"`
DeletedAccounts AccountDiffsMap `json:"deletedAccounts" gencodec:"required"`
UpdatedAccounts AccountDiffsMap `json:"updatedAccounts" gencodec:"required"`
BlockNumber *big.Int `json:"blockNumber" gencodec:"required"`
BlockHash common.Hash `json:"blockHash" gencodec:"required"`
CreatedAccounts []AccountDiff `json:"createdAccounts" gencodec:"required"`
DeletedAccounts []AccountDiff `json:"deletedAccounts" gencodec:"required"`
UpdatedAccounts []AccountDiff `json:"updatedAccounts" gencodec:"required"`
encoded []byte
err error
@@ -81,46 +82,19 @@ func (sd *StateDiff) Encode() ([]byte, error) {
return sd.encoded, sd.err
}
// AccountDiffsMap is a mapping of keccak256(address) => AccountDiff
type AccountDiffsMap map[common.Hash]AccountDiff
// AccountDiff holds the data for a single state diff leaf node
// AccountDiff holds the data for a single state diff node
type AccountDiff struct {
Key []byte `json:"key" gencodec:"required"`
Value []byte `json:"value" gencodec:"required"`
Proof [][]byte `json:"proof" gencodec:"required"`
Storage []StorageDiff `json:"storage" gencodec:"required"`
Path []byte `json:"path" gencodec:"required"`
Storage []StorageDiff `json:"storage" gencodec:"required"`
}
// StorageDiff holds the data for a single storage diff leaf node
// StorageDiff holds the data for a single storage diff node
type StorageDiff struct {
Key []byte `json:"key" gencodec:"required"`
Value []byte `json:"value" gencodec:"required"`
Proof [][]byte `json:"proof" gencodec:"required"`
Path []byte `json:"path" gencodec:"required"`
}
/*
// State trie leaf is just a short node, below
// that has an rlp encoded account as the value
// SO each account diffs map is reall a map of shortnode keys to values
// Flatten to a slice of short nodes?
// Need to coerce into:
type TrieNode struct {
// leaf, extension or branch
nodeKind string
// If leaf or extension: [0] is key, [1] is val.
// If branch: [0] - [16] are children.
elements []interface{}
// IPLD block information
cid *cid.Cid
rawdata []byte
}
*/
1