-
Notifications
You must be signed in to change notification settings - Fork 111
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
[C-313] Refactor EthContracts to typescript #3063
Conversation
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.
Generally not sure between the different of type Wallet from ethereumjs-wallet and type string and when to differentiate the two.
A lot of places don't match the ABIs for type BN vs number - but I'm not sure if that's an issue or if the method should accept both.
Looks great otherwise!
export {} | ||
// const DiscoveryProvider = require('.') | ||
// const helpers = require('../../../tests/helpers') | ||
// const { DISCOVERY_PROVIDER_TIMESTAMP } = require('./constants') | ||
// |
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.
what is this file?
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.
oh good call, will uncomment!
@@ -43,14 +65,26 @@ class AudiusTokenClient { | |||
|
|||
/* ------- SETTERS ------- */ | |||
|
|||
async transfer (recipient, amount) { | |||
const contractMethod = this.AudiusTokenContract.methods.transfer(recipient, amount) | |||
async transfer(recipient: string, amount: number) { |
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 this amount is type BN instead of number
owner: Wallet, | ||
recipient: string, | ||
relayer: Wallet, |
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.
Not sure what the type Wallet exactly is, but I believe this should be able to accept type string for all of these params
r: BinaryData, // bytes32 | ||
s: BinaryData // bytes32 |
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.
Not sure what BinaryData is, but I believe this is of type Uint8Array or Buffer
owner: Wallet, // address | ||
spender: Wallet, // 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.
I think string also works here
@@ -92,8 +126,11 @@ class AudiusTokenClient { | |||
|
|||
// Allow spender to withdraw from calling account up to value amount | |||
// https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20.md | |||
async approve (spender, value, privateKey = null) { | |||
const contractMethod = this.AudiusTokenContract.methods.approve(spender, value) | |||
async approve(spender: string, value: number, privateKey = null) { |
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 believe value should be BN
owner: Wallet, | ||
spender: string, | ||
value: number, |
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 owner can be string and value is BN
proposalId, | ||
queryStartBlock = 0 | ||
) { | ||
async getProposalSubmission(proposalId: number, queryStartBlock = 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.
Technically, I believe proposalId should be a type BN throughout, but I don't think it'll ever get that big to cause an issue, so int should be fine
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.
If you want to know how I check these things, you can look at the ABI (Application Binary Interface) that's outputted by the contract compliation step.
We import these ABI to know how to encode methods to send to the RPC node to communicate, but they have the "types" of all the input param and return types.
ie. Looking here: uint256 is technically type BN, address is type string, bytes32 is type Uint8Array, (I'm fairly sure) etc.
audius-protocol/libs/eth-contracts/ABIs/GovernanceUpgraded.json
Lines 389 to 464 in 82bac74
{ | |
"constant": true, | |
"inputs": [ | |
{ | |
"internalType": "uint256", | |
"name": "_proposalId", | |
"type": "uint256" | |
} | |
], | |
"name": "getProposalById", | |
"outputs": [ | |
{ | |
"internalType": "uint256", | |
"name": "proposalId", | |
"type": "uint256" | |
}, | |
{ | |
"internalType": "address", | |
"name": "proposer", | |
"type": "address" | |
}, | |
{ | |
"internalType": "uint256", | |
"name": "submissionBlockNumber", | |
"type": "uint256" | |
}, | |
{ | |
"internalType": "bytes32", | |
"name": "targetContractRegistryKey", | |
"type": "bytes32" | |
}, | |
{ | |
"internalType": "address", | |
"name": "targetContractAddress", | |
"type": "address" | |
}, | |
{ | |
"internalType": "uint256", | |
"name": "callValue", | |
"type": "uint256" | |
}, | |
{ | |
"internalType": "string", | |
"name": "functionSignature", | |
"type": "string" | |
}, | |
{ | |
"internalType": "bytes", | |
"name": "callData", | |
"type": "bytes" | |
}, | |
{ | |
"internalType": "enum Governance.Outcome", | |
"name": "outcome", | |
"type": "uint8" | |
}, | |
{ | |
"internalType": "uint256", | |
"name": "voteMagnitudeYes", | |
"type": "uint256" | |
}, | |
{ | |
"internalType": "uint256", | |
"name": "voteMagnitudeNo", | |
"type": "uint256" | |
}, | |
{ | |
"internalType": "uint256", | |
"name": "numVotes", | |
"type": "uint256" | |
} | |
], | |
"payable": false, | |
"stateMutability": "view", | |
"type": "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.
ahh this is prefect thank you, was just doing best guess
serviceTypeMin: string, | ||
serviceTypeMax: string, |
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.
These should be type BN - ref:
audius-protocol/libs/eth-contracts/ABIs/ServiceTypeManager.json
Lines 115 to 139 in 82bac74
{ | |
"constant": false, | |
"inputs": [ | |
{ | |
"internalType": "bytes32", | |
"name": "_serviceType", | |
"type": "bytes32" | |
}, | |
{ | |
"internalType": "uint256", | |
"name": "_serviceTypeMin", | |
"type": "uint256" | |
}, | |
{ | |
"internalType": "uint256", | |
"name": "_serviceTypeMax", | |
"type": "uint256" | |
} | |
], | |
"name": "addServiceType", | |
"outputs": [], | |
"payable": false, | |
"stateMutability": "nonpayable", | |
"type": "function" | |
}, |
Thanks for the review! updating |
[e804aa1] [C-2248, C-2373] Use playlistUpdates, remove legacyNotifications (#3094) Dylan Jeffers [824933e] [C-2366] Improve web notification selection performance (#3103) Dylan Jeffers [4b8edef] [PLAT-696] Add trending-playlists/underground notifications (#3089) Dylan Jeffers [1f9cf3e] [C-2275] Fix android drawer offsets (#3095) Dylan Jeffers [fc14c82] [PAY-1063][PAY-1085][PAY-1086] Update UI for inaccessible gated tracks from favorites and history pages (#3100) Saliou Diallo [b0441f5] [C-2365] Update play buttons on web and mobile to show resume when track is current (#3101) Kyle Shanks [453910f] [C-2378] Add upload v2 feature flag (#3099) Sebastian Klingler [962a6df] [C-2337] Remove reachability mobile web (#3090) Raymond Jacobson [4ad5cd2] Fix visible collectibles for upload popup (#3093) Saliou Diallo [c143078] Fix feature flag bug (#3092) Saliou Diallo [44435b5] Fix upload prompt modal learn more url (#3091) Saliou Diallo [c9024ad] Use chat.messagesStatus instead of selector (#3087) Reed [38d43c4] [C-2369] Fix issue where notification poll can break app on signout (#3088) Dylan Jeffers [90122d9] [PAY-923] DMs: Add desktop entrypoints (#3083) Marcus Pasell [00f27e8] [PAY-907] Mobile chat reactions (#3020) Reed [4678b89] DMs: Fix broken typecheck on main (#3086) Marcus Pasell [756ade4] [PAY-1000][PAY-1084][PAY-1096][PAY-1097][PAY-1098] - More gated content fixes (#3085) Saliou Diallo [820aa9d] Fix upload and repost probers tests and lint (#3076) Sebastian Klingler [345607e] [C-2320] Fix profile socials alignment (#3079) Dylan Jeffers [569199c] Fix prod build timeout (#3084) Sebastian Klingler [12f6c22] Remove ports for local dev (#3082) Theo Ilie [1940618] Fix broken Main build due to typeerror (#3080) Marcus Pasell [eb8d47e] [PAY-1082] DMs: Dedupe sent messages (#3066) Marcus Pasell [50a11c3] Update SDK to 2.0.3-beta.0 (#3078) Marcus Pasell [c420fbb] Clean up NPM package lock (#3077) Marcus Pasell [35d1124] [C-2327] Add playlist updates slice (#3063) Dylan Jeffers [59862ad] [C-2344] Update the web playbar scrubber to respect the playback speed of podcasts (#3075) Kyle Shanks [ffeb0d3] [C-2349] Default download on wifi only to false (#3074) Andrew Mendelsohn [cafae41] [C-2325] Fix playlist table date-added column (#3073) Dylan Jeffers [384a510] [PAY-927] DMs: Empty messages state (#3068) Marcus Pasell [1132f83] Update @jup-ag/core to 2.0.0-beta.9 (#3072) Marcus Pasell [49c0ebf] [PAY-1072] Change "Download App" icon on Settings Page (#3067) Marcus Pasell [928dcaf] [PAY-1056] - More gated content updates and fixes (#3070) Saliou Diallo [1e1f769] [C-2345] Move PlaybackRate drawer to common drawers map (#3071) Kyle Shanks [f5d1251] Fix web-dist CI steps (#3069) Sebastian Klingler [5f89800] Fix heavy rotation playlist on client (#3056) sabrina-kiam [c0191e2] [C-2316] Add remote config for all oauth verification (#3052) Raymond Jacobson [40f5627] [PAY-1074][PAY-1075][PAY-1076][PAY-1080] - Update availability settings states + more QA fixes (#3059) Saliou Diallo [5be60ac] [C-2339] Update podcast control updates to also work for audiobooks (#3065) Kyle Shanks [163ebf5] [C-2297] Add fallback flag to podcast feature (#3064) Sebastian Klingler [f206391] [PAY-904] - Add gated content upload prompt (#3057) Saliou Diallo [1afc4e5] [C-1344] Move probers to monorepo and make tests pass (#3061) Sebastian Klingler [e198279] Remove random line (#3062) Saliou Diallo [24a001b] Add playback position logic for mobile (#3051) Kyle Shanks [d210124] [PAY-1070] Update TabSlider/SegmentedControl slider size on resize (#3044) Marcus Pasell
Description
Refactors EthContracts to typescript
Tests
Test pass as before