Skip to content
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

feat: WM on Test wallet #1427

Closed
wants to merge 66 commits into from
Closed

Conversation

sanducb
Copy link
Contributor

@sanducb sanducb commented Jun 27, 2024

Context

This PR introduces changes that will allow creation of asset scale 9 wallet addresses by default and all WM accounting logic is removed as there will be no difference between payment pointers.
Rapyd will only take into account asset scale 2 (this is unchanged) while we also keep track of an asset scale 9 "virtual" balance in our wallet's db. This virtual balance is separated into incomingBalance and outgoingBalance for each PP. Whenever scale 9 incoming or outgoing transfers add up to the lowest amount representable in scale 2 (i.e. 0.01), then Rapyd balance is updated. This is similar to the previous WM accounting logic, but it now applies to all transactions.

Changes TLDR;

  • Allowing creation of asset scale 9 wallet addresses by default
  • Deleted WM accounting logic, wallet addresses are WM ready by default
  • Added virtual balance logic for dealing with scale 9 amounts

Important questions

  • Should we drop incomingBalance and outgoingBalance columns from walletAddresses table and move them to accounts table?
  • Should we drop debt column in the accounts table?

@sanducb sanducb linked an issue Jun 27, 2024 that may be closed by this pull request
2 tasks
@github-actions github-actions bot added type: documentation Improvements or additions to documentation package: wallet/frontend Wallet frontend implementations package: wallet/backend Wallet backend implementations type: test Improvements or additions to tests type: source Source changes labels Jun 27, 2024
@sanducb sanducb changed the title WM on Test wallet feat: WM on Test wallet Jun 27, 2024
@@ -54,7 +54,7 @@ services:
RATE_API_KEY: ${RATE_API_KEY}
BASE_ASSET_SCALE: 2
MAX_ASSET_SCALE: 9
WM_THRESHOLD: 100000000
RAPYD_THRESHOLD: 10000000 # 0.01
Copy link
Contributor Author

@sanducb sanducb Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified this to 0.01 (which is the lowest non-zero amount representable in asset scale 2) instead of 0.1. Any reason to change it back?

@@ -289,7 +289,7 @@ export class RapydClient implements IRapydClient {
: ','
const [street, city, postCode] = args.user.address?.split(', ') ?? []
let address = [street, postCode].join(addressDelimiter)
if (args.assetCode === 'EUR') address = address.replace(/[,\s]+/g, '')
if (args.assetCode === 'USD') address = address.replace(/[,\s]+/g, '')
Copy link
Member

@beniaminmunteanu beniaminmunteanu Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this hardcoded transformation happens here tbh, but why was the asset code change needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also interested in the answer here, we are creating default USD account for every new user in Rapyd, but don't know if that ihas anything to do with this. But why change EUR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I first started working on this issue I couldn't create an EUR account in the onboarding and thought that it should be USD by default. This was a change that I introduced in one of the first commits. Since then, I realised that I was wrong about the onboarding but forgot to revert the changes. Now I switched back to EUR 👍

@@ -387,6 +345,7 @@ export class WalletAddressService implements IWalletAddressService {

if (transfer.status?.status !== 'SUCCESS') {
if (type === 'OUTGOING') {
//TODO this might not be needed
Copy link
Member

@beniaminmunteanu beniaminmunteanu Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as in: debt being part of the walletAddress model might not be needed?

Copy link
Contributor Author

@sanducb sanducb Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I'm not 100% sure we should delete it. It makes sense to have it in case transferring liquidity to Rapyd fails, but also there's no mechanism to handle debt so I assume we don't care if transferring fails since the amounts are very small?

Copy link
Member

@beniaminmunteanu beniaminmunteanu Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point you are raising is valid. I think there should be a retry mechanism in place.

But I think the retry mechanism can be part of the current flow:
Transfer to rapyd fails -> the imbalance state remains the same, and the transfer should be picked up by the next iteration of the worker job and retried, since it should again be detected as imbalance.

This way, no additional mechanism requiring debt would be needed.

if (
!accountsResponse.success ||
!userResponse.success ||
!accountsResponse.result
Copy link
Member

@beniaminmunteanu beniaminmunteanu Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a quick glance, accountResponse.result might end up being an empty list.
Taking the following js truthy behaviour into account:
image

This might not be the desired outcome of the condition^^
Double check if this can be the case, and if such, check for accountsResponse.result.length to reach desired condition

account.id,
ctx.req.headers.cookie
)
if (walletAddressesResponse.success && walletAddressesResponse.result) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto above

README.md Outdated Show resolved Hide resolved
Tymmmy
Tymmmy previously approved these changes Aug 22, 2024
if (!accountsResponse.result) {
return {
notFound: true
const incomingOutgoingBalances: Record<string, number> = {}
Copy link
Contributor Author

@sanducb sanducb Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How could we share incoming/outgoing balance state so that we don't calculate it in each page as we do here (also in send, index)? @Tymmmy @raducristianpopa

@@ -20,6 +20,20 @@ module.exports = {
}
},

rafiki_backend: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this added?

Copy link
Contributor Author

@sanducb sanducb Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this so that I can migrate assets to scale 9 in the assets table which is in rafiki_backend db. That requires a separate connection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add an env variable instead with the connection string

if (account) {
await knex('walletAddresses')
.where('id', walletAddress.id)
.update({ assetCode: account.assetCode })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would asset code change?

Copy link
Contributor Author

@sanducb sanducb Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens because assetCode was null for every WA for some reason and that was causing keepBalancesSynced to fail when checking that asset code exists for every WA. Until now, we only populated assetCode column on WAs table for WM payment pointers. Therefore, I changed the asset code from null to whatever asset code the account has when migrating to prevent issues.

@Tymmmy
Copy link
Contributor

Tymmmy commented Sep 2, 2024

We will cherry pick the useful code we need from this.
Integrating Gatehub sandbox and replacing Rapyd, sadly made this story n0t needed anymore, as only asset code 2 will be supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: wallet/backend Wallet backend implementations package: wallet/frontend Wallet frontend implementations type: documentation Improvements or additions to documentation type: source Source changes type: test Improvements or additions to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BE] Asset scale 2 to asset scale 9 [FE] Asset scale 2 to asset scale 9
5 participants