BRC20 Swap Module Indexer #58
Conversation
| } | ||
| }) | ||
|
|
||
| app.get('/v1/brc20/get_current_balance_of_wallet_2', async (request, response) => { |
There was a problem hiding this comment.
How is this different from get_current_balance_of_wallet? It might be better to add some comments explaining the difference
_2 was a bit confusing for me, is there a /v1/brc20/get_current_balance_of_wallet endpoint too?
|
|
||
| // Decimal represents a fixed-point decimal number with 18 decimal places | ||
| type Decimal struct { | ||
| Precition uint |
There was a problem hiding this comment.
Is there a library we can use for these? I'm wary of adding all this logic in the module itself for maintenance purposes. Also, some methods in this file don't have tests and there's some bugs it looks like, added some comments in individual functions.
This also needs to be better documented and tested, as it's highly crucial for the rest of the module.
| network = bitcoin.networks.regtest; | ||
| } | ||
|
|
||
| const pkscript_to_address = {}; |
There was a problem hiding this comment.
Do we need to memorise all these? Would this cause memory bloat over a long period of time? Same below
| @@ -0,0 +1,340 @@ | |||
|
|
|||
| CREATE TABLE public.brc20_ticker_info ( | |||
There was a problem hiding this comment.
Makes sense to prefix these with brc20_swap to make it clear these are consumed/written by this module, so we don't have conflicts with different modules
| return &Decimal{Precition: d.Precition, Val: value} | ||
| } |
There was a problem hiding this comment.
This seems wrong, as precision can't be derived just from the first one.
Seems like this needs more tests, similar to addition and subtraction.
There was a problem hiding this comment.
For convenience, the decimal places of the first argument are inherited directly when performing multiplication and division. This way, the arguments and results do not need to be re-adjusted to the same decimal places. However, addition and subtraction require strict consistency in the decimal places. We may be able to improve this and make the behavior of the decimal library more independent.
There was a problem hiding this comment.
It's the same with addition. A future contributor may not know how the custom decimal library works so it should do what its signature says in all cases. Using a well-documented and tested decimal library solves this issue.
| } | ||
| if feeRateSwapAmt.Sign() > 0 { | ||
| // lp = (poolLp * (rootK - rootKLast)) / (rootK * 5 + rootKLast) | ||
| rootK := pool.TickBalance[token0Idx].Mul(pool.TickBalance[token1Idx]).Sqrt() |
There was a problem hiding this comment.
Not sure if I'm missing anything but rootK here seems like it will be the same as lastRootK, because the tick balances didn't change in-between, making (rootK - rootKLast) always 0.
There was a problem hiding this comment.
Please check the logic of commit_swap.go. Because the actual amount is adjusted according to the fee when the user swaps, rootK has a slight change compared to the case when no one swaps.
| value := new(big.Int).Set(other.Val) | ||
| return &Decimal{Precition: other.Precition, Val: value} | ||
| } | ||
| if d.Precition != other.Precition { |
There was a problem hiding this comment.
This function should be able to handle this case by using the highest precision
There was a problem hiding this comment.
The Decimal library is closely related to the brc20 logic. Currently, there seems to be no scenario for adding the amounts of different tickers.
There was a problem hiding this comment.
Since this is an open-source project, a future contributor may not know this and may expect the decimal library to just work. So, every function should do what its signature says. In this case, the signature is to add 2 decimal values so it should work on all cases.
It would be much better to use a standard and easy-to-use library instead of a custom decimal library.
| return nil | ||
| } | ||
| // value0 := new(big.Int).Mul(d.Value, precisionFactor[d.Precition]) | ||
| value := new(big.Int).Sqrt(d.Val) |
There was a problem hiding this comment.
This needs better testing too, this works only for integers, not decimals.
Did a little testing here and NewDecimalFromString("100", 18).Sqrt() returns 0.00000001
There was a problem hiding this comment.
Yes, currently sqrt is only performed after multiplying two numbers. The swap module needs a function that performs sqrt after multiplication instead of a separate sqrt.
| if d == nil { | ||
| return -other.Val.Sign() | ||
| } | ||
| if d.Precition != other.Precition { |
There was a problem hiding this comment.
This also should be able to handle this case by converting one to the other
Preliminary implementation of the brc20 swap module.