-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/integrate lisk #63
Conversation
50fa2fa
to
e7e397e
Compare
11bad92
to
98341b8
Compare
98341b8
to
f731212
Compare
chains/lisk/client.go
Outdated
} | ||
|
||
var responseObject types.ResponseWrapper | ||
json.Unmarshal(responseData, &responseObject) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json.Unmarshal returns error. Make sure you catch that error here.
chains/lisk/client.go
Outdated
latestBlock := blocks[0] | ||
latestBlockJson, _ := json.Marshal(latestBlock) | ||
var block types.Block | ||
if err := json.Unmarshal(latestBlockJson, &block); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do
block, ok := latestBlock.(*types.Block)
chains/lisk/client.go
Outdated
return nil, err | ||
} | ||
var block types.Block | ||
if err := json.Unmarshal(latestBlockJson, &block); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
block, ok := latestBlock.(*types.Block)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im using a map so we can't convert from map to interface
When I apply your code , I got this error invalid operation: latestBlock (variable of type map[string]interface{}) is not an interface
chains/lisk/client.go
Outdated
} | ||
var transactions []types.Transaction | ||
if err := json.Unmarshal(transactionsJson, &transactions); err != nil { | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated err checking
chains/lisk/test/main.go
Outdated
) | ||
|
||
func testWatcher() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed
return ret | ||
} | ||
func (w *Watcher) acceptTx(tx types.Transaction) bool { | ||
if tx.Asset.Recipient.Address != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to do nil checking here for tx.Asset
and tx.Asset.Recipient
or the app will crash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed.
chains/lisk/test/main.go
Outdated
@@ -0,0 +1,43 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am moving away from creating a test folder for integration test.
Can you create a watcher_integration_test.go
and put this test into that file? The test should have prefix TestIntegration_...
After you are done with the test, add test.Skip()
so that the CI system will skip the test
You need to write test for the watcher. The test should cover at least 2 basic cases: The watcher should return a list of txs on a new block with interested txs and does not return when there is no tx going through watched address (though there are still txs in the block). You can use in-memory db using |
chains/lisk/mock.go
Outdated
ethtypes "github.com/ethereum/go-ethereum/core/types" | ||
) | ||
|
||
type MockEthClient struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you mock this for lisk?
return 0, err | ||
} | ||
blocks := responseObject.Data | ||
latestBlock := blocks[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check length of blocks before doing this.
chains/lisk/watcher.go
Outdated
) | ||
txFormatted := ctypes.Tx{ | ||
Hash: tx.Id, | ||
Serialized: []byte(tx.Signatures[0]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check signatures length here
chains/lisk/watcher.go
Outdated
log.Info(w.cfg.Chain, " Block length = ", block.NumberOfTransactions) | ||
txArr := make([]*ctypes.Tx, 0) | ||
for _, tx := range block.Transactions { | ||
if strings.EqualFold(tx.Sender.Address, w.vault) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong checking. We want to check the recipient of the asset, not the sender.
No description provided.