-
Notifications
You must be signed in to change notification settings - Fork 89
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
fix(backend): make the rafiki wallet url case insensitive #2833
fix(backend): make the rafiki wallet url case insensitive #2833
Conversation
…es/backend/src/graphql/resolvers/wallet_address.ts). middleware.ts in packages/backend/src/open_payments/wallet_address/middleware.ts modified to look up for the walletaddress url in a case insensitive way.
✅ Deploy Preview for brilliant-pasca-3e80ec canceled.
|
@s100tist CI prerequisite job is failing due to code format. Please run |
…0tist/rafiki into backend-case-insensitiveness
@@ -15,23 +15,24 @@ export async function getWalletAddressUrlFromRequestBody( | |||
ctx: SignedCollectionContext<CreateBody>, | |||
next: () => Promise<void> | |||
) { | |||
ctx.walletAddressUrl = ctx.request.body.walletAddress | |||
ctx.walletAddressUrl = ctx.request.body.walletAddress.toLowerCase() |
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.
Instead of doing this all over, we can just do .toLowerCase
in during creation (walletAddressService.create
) and when we fetch it by url (getWalletAddressByUrl
)
ie, as close to the DB as possible
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 I correctly understood it would be better to discard the packages/backend/src/open_payments/wallet_address/middleware.ts
and packages/backend/src/graphql/resolvers/wallet_address.ts
changes and better lowercase inside packages/backend/src/open_payments/wallet_address/service.ts
?
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.
@mkurapov maybe going a little bit further, but probably one very neat solution would be to use GraphQL directive for this kind of things?
Our input object would then look like this
input CreateWalletAddressInput {
"Asset of the wallet address"
assetId: String!
"Wallet Address URL"
url: String! @lower
"Public name associated with the wallet address"
publicName: String
"Unique key to ensure duplicate or retried requests are processed only once. See [idempotence](https://en.wikipedia.org/wiki/Idempotence)"
idempotencyKey: String
"Additional properties associated with the [walletAddress]."
additionalProperties: [AdditionalPropertyInput!]
}
Notice that @lower
? With this kind of directive we would make sure that we always get lower case url in the request.
So the only question would be getWalletAddressByUrl
right? If I am not mistaken this gets called only when we create receiver right or with other words, it is only getting called from GraphQL not Rest right? In that case, we can also add directive to that field.
Instead of doing this thing manually as close to the database as possible we would have lowercase url since the beginning and it would be the same as if user were inputing url in lower case.
But maybe this is too much for this ticket?
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's quite a nice solution actually, and we probably have a few opportunities where we could leverage GQL directives more.
For this case though, I think it makes sense to keep this in the service/domain layer because no matter the interface/API used to create wallet addresses, we always have this baked into the logic before we create the 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.
@tadejgolobic would this change not be retroactive? I mean the already created wallets will not be found.
Assuming that you mean admin API instead of GraphQL. Yes, the getWalletAddressByUrl
is geting called from the admin API and the rest (open payments>get method)
packages/backend/src/open_payments/wallet_address/middleware.ts
Outdated
Show resolved
Hide resolved
@@ -166,7 +166,7 @@ async function createWalletAddress( | |||
|
|||
return await WalletAddress.query(deps.knex) | |||
.insertGraphAndFetch({ | |||
url: options.url, | |||
url: options.url.toLowerCase(), |
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 add some tests for these :)
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.
@s100tist
in the catch statement, you can check for err instanceof UniqueViolationError
to catch those cases where you are trying to create a duplicate wallet address
packages/backend/src/open_payments/wallet_address/service.test.ts
Outdated
Show resolved
Hide resolved
packages/backend/src/open_payments/wallet_address/service.test.ts
Outdated
Show resolved
Hide resolved
packages/backend/src/open_payments/wallet_address/service.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Max Kurapov <[email protected]>
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.
🚀
Changes proposed in this pull request
Converting the wallet address to lower case when it get stored
Making the getWalletAddres middleware methods looking for a wallet addres url converted to lower case
Context
When a wallet address is created, the wallet address gets stored in the database as it was written originally.
This causes undesired beheaviour such as 'wallet address not found' being raised due to the case sensitiveness.
This PR fixes issue #2815
Checklist
fixes #number