migrate: swaps #468
migrate: swaps #468
Conversation
Migrate Token Swap features from assets-chaincode to public sdk From the beginning, at least the start of my time on November 1, 2021, GalaChain has planned to support and included an implementation of peer-to-peer token swapping. This feature has been migrated, updated, and refined through many refactorings and changes in chaincode architecture. Many hands contributed to this feature. It is only now getting migrated to the public sdk for wider use, due to various competing demands throughout 2024. Finally, with this commit, the remainder of the swap implementation will be provided with the developer sdk.
|
Related to #443, item 8) |
| ): Promise<TokenSwapFill[]> { | ||
| const params = dto.swapDtos.map((d) => ({ | ||
| swapRequestId: d.swapRequestId, | ||
| filledBy: asValidUserAlias(d.filledBy ?? ctx.callingUser), |
There was a problem hiding this comment.
we should use await resolveUserAlias(ctx, d.filledBy). The case is resolveUserAlias searcher in chain state if a given user is registered. This way you can provide valid user ref as filledBy, and it will find proper alias (like it will find client|abc for some provided eth address)
There was a problem hiding this comment.
Also I think it's worth checking if the following pattern applies to swap features:
- For DTOs use
UserRef - Do the
resolveUserAliasin contract methods - For swap functions params use
UserAlias(unambiguous)
For instance a few lines below there is:
return fetchTokenSwapsOfferedByUser(ctx, { ...dto, user: dto.user ?? ctx.callingUser });Which suggests that dto.user is UserAlias, and it could be UserRef, and the function call could be something like:
return fetchTokenSwapsOfferedByUser(ctx, { ...dto, user: dto.user ? await resolveUserAlias(ctx, dto.user) : ctx.callingUser });| function acceptNotFoundError(e: Error) { | ||
| const chainError = ChainError.from(e); | ||
| if (chainError.matches(ErrorCode.NOT_FOUND)) { | ||
| return null; | ||
| } else { | ||
| throw chainError; | ||
| } | ||
| } |
There was a problem hiding this comment.
We have a built it function for that and the call:
acceptNotFoundError(e)can be replaced with:
ChainError.ignore(e, ErrorCode.NOT_FOUND)`| let noOp = true; | ||
| const writes: ChainObject[] = []; |
There was a problem hiding this comment.
isn't noOp an equivalent of !writes.length?
There was a problem hiding this comment.
Technically yes, it appears that way.
However I'm not sure if GalaSwap/Connect or other consumers are relying on this boolean being present in the response, so I would be hesitant to change it without looking into that.
The scope of the initial task was just to migrate code as-is, so I've left this as I found it.
| let tokenSwap: TokenSwapRequest; | ||
|
|
||
| try { | ||
| tokenSwap = await getObjectByKey(ctx, TokenSwapRequest, swapRequestId); | ||
| } catch (error) { | ||
| throw new Error(`Token swap with swapRequestId ${swapRequestId} not found.`); | ||
| } | ||
|
|
||
| return tokenSwap; |
There was a problem hiding this comment.
| let tokenSwap: TokenSwapRequest; | |
| try { | |
| tokenSwap = await getObjectByKey(ctx, TokenSwapRequest, swapRequestId); | |
| } catch (error) { | |
| throw new Error(`Token swap with swapRequestId ${swapRequestId} not found.`); | |
| } | |
| return tokenSwap; | |
| return await getObjectByKey(ctx, TokenSwapRequest, swapRequestId); |
why:
- The error thrown indicates "not found" even if the underlying error in
getObjectByKeyis different Errormisses some useful properties thatChainError(thrown fromgetObjectByKey) has, like mapping to http 404.
I also don't think we need a custom error type for that function
| const swapRequestIds = Array.from( | ||
| new Set( | ||
| instancesOffered.results.map((instanceOffered) => { | ||
| return instanceOffered.swapRequestId; | ||
| }) | ||
| ) |
There was a problem hiding this comment.
| const swapRequestIds = Array.from( | |
| new Set( | |
| instancesOffered.results.map((instanceOffered) => { | |
| return instanceOffered.swapRequestId; | |
| }) | |
| ) | |
| const swapRequestIds = Array.from(new Set(instancesOffered.results.map((i) => i.swapRequestId)); |
| // todo: unsure why verifyAndUseAllowances() requires *both* the tokenInstanceKey and tokenInstance be supplied - | ||
| // its code appears to just need the string properties not the actual TokenInstance off chain | ||
| // that might be worth reworking in the future |
There was a problem hiding this comment.
could you gather all of those problematic TODOs, and create a new issue for them? I believe it's a good way of enforcing more thorough review (by doing) of the business logic behind swaps
There was a problem hiding this comment.
Agree, I have written up a new issue here:
It summarizes both high level goals of a review/refactor given new features introduced in the SDK since it was originally written, and it summarizes your comments on this pull request into a bulleted list.
| // 2023-11-16: added txid as new chain key because timestamp alone may not be precise enough to ensure uniqueness | ||
| // todo: determine if it would be straightforward to remove created as a ChainKey() from the TokenSwapRequest object | ||
| // i.e. if this feature has never been used in production, it could be easy |
There was a problem hiding this comment.
probably this comment needs some update
Migrate Token Swap features from assets-chaincode to public sdk
From the beginning, at least the start of my time on November 1, 2021, GalaChain has planned to support and included an implementation of peer-to-peer token
swapping.
This feature has been migrated, updated, and
refined through many refactorings and
changes in chaincode architecture. Many hands
contributed to this feature. It is only
now getting migrated to the public sdk for
wider use, due to various competing demands
throughout 2024. Finally, with
this commit, the remainder of the swap
implementation will be provided with the developer sdk.