-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Auto-detect tokens #3034 #4683
Auto-detect tokens #3034 #4683
Conversation
b0af372
to
63b9c87
Compare
* | ||
* @param {Object} [config] - Options to configure controller | ||
*/ | ||
constructor ({ interval = DEFAULT_INTERVAL, preferences, network } = {}) { |
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.
contracts
is lost
const address = this._preferences.store.getState().selectedAddress | ||
const response = await fetch(`https://api.etherscan.io/api?module=account&action=tokenbalance&contractaddress=${contractAddress}&address=${address}&tag=latest&apikey=NCKS6GTY41KPHWRJB62ES1MDNRBIT174PV`) | ||
const parsedResponse = await response.json() | ||
if (parsedResponse.result !== '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.
the theory is good, if result > 0
you have tokens
but for real case
{"status":"1","message":"OK","result":"8121799999999999999558"}
i have 8121 tokens, because contract code says
uint8 public decimals = 18;
so let say i have
{"status":"1","message":"OK","result":"1000"}
1000 > 0 but it still means nothing because 1000 * 1e-18 = 1e-15 = 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.
1e-15 = 0
is a false statement, we can disregard this comment.
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.
Just some cleanups that'll make it a bit stronger, thanks so much for doing this!
for (const address in this.contracts) { | ||
const contract = this.contracts[address] | ||
if (contract.erc20 && !(address in this.tokens)) { | ||
detectedTokenAddress = await this.fetchContractAccountBalance(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.
Should probably be named detectedTokenBalance
.
} | ||
} | ||
// etherscan restriction, 5 request/second, lazy scan | ||
setTimeout(() => {}, 200) |
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.
This is non-blocking. you probably want to do something like:
await new Promise((res, rej) => { setTimeout(res, 200) })
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.
Really we want to reset the interval here, so we want to do something more like:
await new Promise((res, rej) => { setTimeout(res, 200) })
this.exploreNewTokens()
*/ | ||
async fetchContractAccountBalance (contractAddress) { | ||
const address = this._preferences.store.getState().selectedAddress | ||
const response = await fetch(`https://api.etherscan.io/api?module=account&action=tokenbalance&contractaddress=${contractAddress}&address=${address}&tag=latest&apikey=NCKS6GTY41KPHWRJB62ES1MDNRBIT174PV`) |
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.
Let's update this to a MetaMask-org API key: ZPN1UTJR48QKJT6GZTJEZYHBMKTK42EPWR
.
const address = this._preferences.store.getState().selectedAddress | ||
const response = await fetch(`https://api.etherscan.io/api?module=account&action=tokenbalance&contractaddress=${contractAddress}&address=${address}&tag=latest&apikey=NCKS6GTY41KPHWRJB62ES1MDNRBIT174PV`) | ||
const parsedResponse = await response.json() | ||
if (parsedResponse.result !== '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.
1e-15 = 0
is a false statement, we can disregard this comment.
const ObservableStore = require('obs-store') | ||
|
||
describe('DetectTokensController', () => { | ||
it('should poll on correct interval', async () => { |
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.
Could use a test to verify that a second call is fired after a second interval.
'decimals': 18, | ||
}, | ||
} | ||
controller.fetchContractAccountBalance = address => 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.
Might be worth using sinon.stub
here, so it can be automatically cleaned up after the test.
'decimals': 18, | ||
}, | ||
} | ||
controller.fetchContractAccountBalance = address => 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.
This method is named a little strange, for returning an address.
'decimals': 18, | ||
}, | ||
} | ||
controller.fetchContractAccountBalance = address => 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.
Here if this was a sinon.stub
, we could assert that it's only been called once.
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.
Sinon example:
const tokenDetectorMock = sinon.mock(controller)
tokenDetectorMock.expects('fetchContractAccountBalance').once()
await controller.exploreNewTokens()
mock.verify()
* For each token in eth-contract-metada, find check selectedAddress balance. | ||
* | ||
*/ | ||
async exploreNewTokens () { |
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 just tried it out. Every three minutes may be fine for a background poll, but if we really want this feature, we should make it responsive.
I think we should also trigger this function from the metamask-controller
when:
submitPassword
is successfully called.- When a new account is selected (It will be something like `this.preferencesController.store.subscribe((state) => /* check for new value here... we should discuss the best way to do this... makes me wish we were using Ember Object... */
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.
It totally makes sense.
Actually in line 80 of detect-tokens.js
there is a selectedAddress
update each time that the address is changed that could be used to trigger this function or also to restart the polling period, what do you think about that?
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.
That would be perfect, thank you!
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.
Ok, I did it in a way that each time that the address is updated or there is a password submitted this function is going to be triggered and the polling is going to be initialized again. All from the detect-tokens
controller to avoid dependency between those controllers.
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.
Also rather than using the keyringController
as a way to detect a submitPasword
event I used the same method already implemented to detect when the extension is active and therefore the user logged in.
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.
needs change on this last item
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.
Looks great! Just add a changelog entry then feel free to merge!
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.
Approving after Changelog commit invalidated Dan's review
Auto-detect tokens feature