This repository has been archived by the owner on Aug 2, 2021. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
swap: chequebook persistence #1797
swap: chequebook persistence #1797
Changes from 37 commits
7f45609
6692bae
a6354de
cf0ba7e
56daf95
6e391f0
0c260c3
632de27
662233f
949c5dc
5d293c6
ac2160e
c148730
419a06f
5f00a9c
7408362
a07c5c1
ec9260d
4b4e3f7
6759d92
b8ff0fc
6b98755
b7f98ac
26d9467
f9e1837
e2e8edc
e414145
170878d
bbf2f9b
a5fd51a
bf2cad0
2e53e83
675c797
2441747
5d7484d
047422c
98771d6
42aaca0
286cf36
27e2220
aca25a2
8b1400d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
as a comment for the function in general: i think all of the network id logic is needed, but it kind of makes the code convoluted. if possible, i would extract all of this logic in a separate function.
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 agree, this whole initialization process is long and not very readable.
I also think it could be compressed. Not well verified but you could check if this works (and if it is correct):
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 put the logic in a separate function
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.
same here: we have too much store/config code here and much less code actually starting the chequebook. i think it's better to separate this if possible
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 had a look and simplified where possible. If you think we can still simplify it, I propose to make an issue to tackle this
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 was this
Info
removed? can we not have this log in thedeployLoop
call, at least?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 would like to know as well
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.
The log, when we deploy a chequebook will be:
auditLog.Info("deploying new swap", "owner", opts.From.Hex(), "deposit", opts.Value)
(line 538)auditLog.Info("Using the chequebook", "chequebookAddr", chequebookToUse)
(line 512)Given that we have these two logs, I thought it would be unnecessary to also have the log
auditLog.Info("swap deployed", "address", address.Hex(), "owner", opts.From.Hex())
If you don't agree, I will change it
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 think that "using the chequebook" isn't very helpful (is the chequebook address in hex?)
First "deploying new..." and then "swap deployed" makes more sense to me. It clearly shows the user that the deployment is being attempted and then that it succeeded.
Maybe we could move "using the chequebook" to a debug log.
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.
Updated.
We now have a log for deploying,
auditLog.Info("Deploying new swap", "owner", opts.From.Hex(), "deposit", opts.Value)
log for deployed
auditLog.Info("Deployed chequebook", "contract address", contract.ContractParams().ContractAddress.Hex(), "deposit", toDeposit, "owner", owner.address)
amd a seperate log for if we are re-using a previously deployed chequebook
auditLog.Info("bound to chequebook", "chequebookAddr", address)