client/asset/eth: less provider check spam #2159
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chappjc
force-pushed
the
eth-log-stuff
branch
from
f9b0467
to
a203a03
Compare
|
Compared to #2158, it now looks like: |
|
The compatibility tests weren't supposed to be logging at all outside of tests ( |
This quiets down the logs from the RPC provider compatibility tests, which was only supposed to be enable for testing and debugging. This also removes a duplicate check for PendingNonceAt. This also removes the redundant sublogger so that CORE[eth][ETH] becomes just CORE[eth]. Same on server. Finally, the changes the subloger used by CreateWallet not be CORE[CREATE], and instead just show the asset symbol.
chappjc
force-pushed
the
eth-log-stuff
branch
from
a203a03
to
3675ba0
Compare
chappjc
commented
Feb 22, 2023
| @@ -697,7 +696,7 @@ func createAndCheckProviders(ctx context.Context, walletDir string, endpoints [] | |||
| if len(providers) != len(unknownEndpoints) { | |||
| return providersErr(providers) | |||
| } | |||
| if err := checkProvidersCompliance(ctx, providers, net, log); err != nil { | |||
| if err := checkProvidersCompliance(ctx, providers, net, dex.Disabled /* logger is for testing only */); err != nil { | |||
There was a problem hiding this comment.
Could do this when calling createAndCheckProviders, but that's only used in product code, whereas the tests use checkProvidersCompliance directly.
Happy to change.
EDIT: There are some logs up there we probably would want.
chappjc
commented
Feb 22, 2023
Comment on lines
+1451
to
+1452
| default: // caller should have checked though | ||
| panic(fmt.Sprintf("Unknown net %v in compatibility tests. Testing data not initiated.", net)) |
There was a problem hiding this comment.
In the product code, there are probably 30 places it would panic before getting here anyway. It's virtually impossible to get an unrecognized dex.Network here with the product where it's checked first thing on app bring-up.
martonp
approved these changes
Feb 22, 2023
|
Just pushed an inconsequential commit to remove "geth" from a few spots where it's no longer right. There was also some needless bloat in |
chappjc
force-pushed
the
eth-log-stuff
branch
from
15ad2fa
to
3428fd9
Compare
chappjc
commented
Feb 22, 2023
| @@ -3947,7 +3939,8 @@ func (w *assetWallet) estimateTransferGas(val uint64) (gas uint64, err error) { | |||
|
|
|||
| // redeem redeems a swap contract. Any on-chain failure, such as this secret not | |||
| // matching the hash, will not cause this to error. | |||
| func (w *assetWallet) redeem(ctx context.Context, assetID uint32, redemptions []*asset.Redemption, maxFeeRate, gasLimit uint64, contractVer uint32, nonceOverride *uint64) (tx *types.Transaction, err error) { | |||
| func (w *assetWallet) redeem(ctx context.Context, assetID uint32 /* ?? */, redemptions []*asset.Redemption, | |||
There was a problem hiding this comment.
I don't know what this input parameter is about. Should it have been used or was this copy pasta from other methods like this?
JoeGruffins
approved these changes
Feb 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves #2158
This also resolves the missing parent path for the compliant providers json file on initial wallet creation.