-
Notifications
You must be signed in to change notification settings - Fork 44
Integrate staking amount requirement #3632
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
base: develop
Are you sure you want to change the base?
Conversation
- Added ExchangeApiKeysModule for managing exchange API keys. - Added validation for exchange names using ExchangeNameValidator. - Integrated AES encryption for storing API keys securely. - Implemented exchange-specific services (GateExchangeService, MexcExchangeService) for interacting with respective APIs. - Enhanced AuthService to check staking eligibility based on exchange balances. - Added configuration services for encryption and staking parameters. - Created migration for the exchange_api_keys table in the database.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 5 Skipped Deployments
|
dnechay
left a 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 a though: it can probably make sense to have just exchanges module where you have all the logic of working with api keys and exchange APIs to avoid circular dependencies and simplify how you instantiate exchange client with api keys of specific user
Review summary
Implementation-wise (i.e. not taking into account code-style/refactoring comments) I have a question regarding of UX of this feature.
Right now in case of errors from exchange (no matter what error: access error or network error) we always fallback to 0 balance. It can lead to situations when some request had a short "hiccup" (dumb network error) and as a result user has "not eligible stake" for the whole JWT lifetime, and for the user it will look like they just don't have any oracles/jobs even if they have necessary staked amount, so that would be very confusing and only thing on how they can understand that is to reach our support, we go to logs, see error and etc. Another case is that they staked after the sing in: they will have to wait until token refresh.
Would be nice to discuss some options of how we can improve this (if we need to, because maybe it's fine to just let them know about "delays"). Maybe instead of having stake_eligible: boolean we can have some sort of stake_status: 'eligible' | 'error' | 'not_eligible' that will help UI to distinguish between different states and to show some hints + ability to trigger refresh. That might be an overhead though, so let's discuss it internally
packages/apps/reputation-oracle/server/src/modules/web3/web3.service.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/common/constants/index.ts
Outdated
Show resolved
Hide resolved
packages/apps/human-app/server/src/modules/jobs-discovery/jobs-discovery.controller.ts
Outdated
Show resolved
Hide resolved
packages/apps/human-app/server/src/modules/oracle-discovery/oracle-discovery.controller.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/auth/auth.service.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/auth/auth.service.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/auth/auth.service.ts
Outdated
Show resolved
Hide resolved
- Introduced ExchangeClientFactory to handle exchange client creation for different exchanges. - Implemented GateExchangeClient and MexcExchangeClient for API interactions. - Removed deprecated ExchangeRouterService and related services. - Updated error handling to include ExchangeApiClientError for better clarity.
dnechay
left a 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.
LGTM overall
- Please change back volume for services as mentioned in the earlier comment
- Let's wait for UI/UX to make sure they are aligned
- Can add unit tests where necessary (e.g. for
auth.service) - Let's try to DRY send request logic in each of exchange clients (i.e. separately for each exchange, not one logic shared across all of them)
packages/apps/reputation-oracle/server/src/modules/web3/web3.service.ts
Outdated
Show resolved
Hide resolved
.../apps/reputation-oracle/server/src/modules/exchange-api-keys/exchange-api-keys.controller.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/scripts/setup-staking.ts
Outdated
Show resolved
Hide resolved
...ges/apps/reputation-oracle/server/src/modules/exchange-api-keys/exchange-api-keys.service.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/encryption/aes-encryption.service.ts
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/exchange/mexc-exchange.client.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/exchange/gate-exchange.client.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/exchange/gate-exchange.client.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/exchange/gate-exchange.client.ts
Outdated
Show resolved
Hide resolved
packages/apps/human-app/server/src/modules/exchange-api-keys/model/exchange-api-keys.model.ts
Show resolved
Hide resolved
…geClient, MexcExchangeClient and exchangeApiKeyService
…ndling utility for improved error handling and code consistency
dnechay
left a 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.
NIce tests coverage!
LGTM overall. Some comments on tests improvements + need to wait for final UI/UX
packages/apps/reputation-oracle/server/src/modules/exchange/fixtures/exchange.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/exchange/utils.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/exchange/mexc-exchange.client.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/exchange/mexc-exchange.client.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/exchange/gate-exchange.client.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/exchange/mexc-exchange.client.spec.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/exchange/mexc-exchange.client.spec.ts
Outdated
Show resolved
Hide resolved
packages/apps/reputation-oracle/server/src/modules/exchange/mexc-exchange.client.spec.ts
Outdated
Show resolved
Hide resolved
…tructure; add signature generation for API requests
| const timestamp = faker.number.int(); | ||
| const client = new MexcExchangeClient({ apiKey, secretKey }); | ||
|
|
||
| jest.spyOn(Date, 'now').mockReturnValue(timestamp); | ||
| const result = client['getSignedQuery'](); | ||
| expect(result).toHaveProperty('query'); | ||
| expect(result).toHaveProperty('signature'); | ||
| expect(result.query).toBe(`timestamp=${timestamp}&recvWindow=5000`); | ||
|
|
||
| const expectedSignature = crypto | ||
| .createHmac('sha256', secretKey) | ||
| .update(result.query) | ||
| .digest('hex'); | ||
| expect(result.signature).toBe(expectedSignature); | ||
| jest.restoreAllMocks(); |
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 correctly mock timers where needed
| const timestamp = faker.number.int(); | |
| const client = new MexcExchangeClient({ apiKey, secretKey }); | |
| jest.spyOn(Date, 'now').mockReturnValue(timestamp); | |
| const result = client['getSignedQuery'](); | |
| expect(result).toHaveProperty('query'); | |
| expect(result).toHaveProperty('signature'); | |
| expect(result.query).toBe(`timestamp=${timestamp}&recvWindow=5000`); | |
| const expectedSignature = crypto | |
| .createHmac('sha256', secretKey) | |
| .update(result.query) | |
| .digest('hex'); | |
| expect(result.signature).toBe(expectedSignature); | |
| jest.restoreAllMocks(); | |
| const client = new MexcExchangeClient({ apiKey, secretKey }); | |
| const now = Date.now(); | |
| jest.useFakeTimers({ now }); | |
| const result = client['getSignedQuery'](); | |
| jest.useRealTimers(); | |
| expect(result).toHaveProperty('query'); | |
| expect(result).toHaveProperty('signature'); | |
| expect(result.query).toBe(`timestamp=${now}&recvWindow=5000`); | |
| const expectedSignature = crypto | |
| .createHmac('sha256', secretKey) | |
| .update(result.query) | |
| .digest('hex'); | |
| expect(result.signature).toBe(expectedSignature); |
| await expect(client.checkRequiredAccess()).rejects.toBeInstanceOf( | ||
| ExchangeApiClientError, | ||
| ); |
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 need to wrap this call to try..catch and check that thrownError is what you expect later like
let thrownError;
try {
await client.checkRequiredAccess()
} catch (error) {
thrownError = error;
}
scope.done();
expect(thrownError).toBeInstanceOf(ExchangeApiClientError);
expect(thrownError.message).toBe('message');
otherwise in case this test fails and scope.done is not called - it can affect other test runs
| .get(path) | ||
| .query(true) | ||
| .replyWithError('network error'); | ||
| await expect(client.getAccountBalance(asset)).rejects.toBeInstanceOf( |
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: must be wrapped into try-catch
| .get(path) | ||
| .query(true) | ||
| .replyWithError('network error'); | ||
| await expect(client.checkRequiredAccess()).rejects.toBeInstanceOf( |
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 be in try-catch
| const path = '/spot/accounts'; | ||
| const query = `currency=${encodeURIComponent(asset)}`; | ||
| const body = ''; | ||
| const ts = String(Math.floor(Date.now() / 1000)); |
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.
Would be nice to move this part with timestamp generation to signGateRequest and return both signature and timestamp for request from it
| const secretKey = faker.string.sample(); | ||
| const client = new GateExchangeClient( | ||
| { apiKey, secretKey }, | ||
| { timeoutMs: 1234 }, |
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.
nit
| { timeoutMs: 1234 }, | |
| { timeoutMs: faker.number.int() }, |
| .get(path) | ||
| .query(true) | ||
| .replyWithError('network error'); | ||
| await expect(client.getAccountBalance(asset)).rejects.toBeInstanceOf( |
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 be in try catch
Issue tracking
Close #3618, #3560, #3619, #3633
Context behind the change
Reputation Oracle:
Human App:
How has this been tested?
In progress
Release plan
Add new
AES_ENCRYPTION_KEYandSTAKING_ELIGIBILITY_ENABLEDin Reputation OraclePotential risks; What to monitor; Rollback plan
Not yet defined