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.

Sign up for GitHub

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

encoding/json: Add custom JSON package with build tag support for Sonic #1623

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open

encoding/json: Add custom JSON package with build tag support for Sonic #1623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
8333e5c
tag optional sonic and allow full library conversion
Aug 20, 2024
c47137c
Add workflow and disallow arm and darwin usage
Aug 20, 2024
6e24f60
Add basic hotswap benchmark
Aug 20, 2024
b70b9e7
linter: fix
Aug 20, 2024
84dd409
use bash
Aug 20, 2024
711db77
linter: fix?
Aug 20, 2024
223f786
Fix whoopsie, add to make file, also add mention in features list.
Aug 20, 2024
9a72688
Merge branch 'master' into encoding_swap
Aug 24, 2024
2f69362
Merge branch 'master' into encoding_swap
Sep 2, 2024
79c85c9
Merge branch 'master' into encoding_swap
Sep 13, 2024
b4f0243
Merge branch 'master' into encoding_swap
Sep 13, 2024
417e0aa
Merge branch 'master' into encoding_swap
Sep 15, 2024
83f1d57
Merge branch 'master' into encoding_swap
Oct 2, 2024
f2d56cf
Merge branch 'master' into encoding_swap
Oct 10, 2024
7bd7cc4
Merge branch 'master' into encoding_swap
Oct 23, 2024
79ebfa1
Merge branch 'master' into encoding_swap
Nov 11, 2024
6b6fb5e
test enforcement
Nov 11, 2024
ac47cc6
actually read documentation see if this works
Nov 11, 2024
135e2b6
linter: fix
Nov 11, 2024
2d08549
Merge branch 'master' into encoding_swap
Nov 25, 2024
1a82764
Merge branch 'master' into encoding_swap
Jan 8, 2025
1ca8eaa
linter: fix
Jan 8, 2025
ab5fd01
sonic: bump tagged version
Jan 10, 2025
1697977
encoding/json: drop build tag arch and os filters
Jan 10, 2025
3905b62
encoding/json: consolidate tests
Jan 10, 2025
1c9f9c2
encoding/json: log build tag usage
Jan 10, 2025
4f8e487
Merge branch 'master' into encoding_swap
Jan 13, 2025
9cffc8f
rm superfluous builds
Jan 13, 2025
1b15e4c
glorious/nits: add template change and regen docs
Jan 28, 2025
a7ac15d
glorious/nits: update commentary on nolint directive
Jan 28, 2025
f23f3c3
glorious/nits: rm init func and log results in main.go
Jan 28, 2025
fba5316
Test to actually pull flag in
Jan 28, 2025
880cd37
Merge branch 'master' into encoding_swap
Feb 2, 2025
dde84c7
Merge branch 'master' into encoding_swap
Feb 11, 2025
c936252
linter: fix
Feb 11, 2025
a843b58
thrasher: nits
Feb 12, 2025
09bac6c
gk: nits 4 goflags goooooooooo!
Feb 13, 2025
a9ce5aa
gk: nits rn
Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,32 @@ jobs:
goarch: amd64
gloriousCode marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

@gloriousCode gloriousCode Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm going to go ahead and leave a code comment because then people can actually reply to eachother instead of big messy texts blocks all over the place 🌈

Before we learned that this json package was compatible with more targets than initially thought, the design and default options for this PR was well considered by shazbert. Now that it has changed, with arm64, linux & mac support, we can actually switch the default to using sonic.
So it wasn't about dipping toes, shazbert had a windows/linux amd64 only pr and ensured compatibility for everyone else. Now we can still use his design to test out other json packages easily in future

So I'd be happy for it to switch, so long as the builds default to sonic enabled with one for sonic disabled to ensure we catch anything improper (so as to not get too dependent on a particular json package) and then decide from there whether we want to be a

psql: true
skip_wrapper_tests: false
sonic: false
- os: ubuntu-latest
goarch: 386
psql: true
skip_wrapper_tests: true
sonic: false
- os: macos-latest
goarch: amd64
psql: true
skip_wrapper_tests: true
sonic: false
- os: macos-13 # beta
goarch: amd64
psql: true
skip_wrapper_tests: true
sonic: false
- os: windows-latest
goarch: amd64
psql: true
skip_wrapper_tests: true
sonic: false
- os: ubuntu-latest
gloriousCode marked this conversation as resolved.
Show resolved Hide resolved
goarch: amd64
psql: true
skip_wrapper_tests: false
sonic: true

runs-on: ${{ matrix.os }}

Expand Down Expand Up @@ -84,6 +94,13 @@ jobs:
echo "CGO_ENABLED=1" >> $GITHUB_ENV
shell: bash

- name: Set GOFLAGS
run: |
if [ "${{ matrix.sonic }}" = "true" ]; then
echo "GOFLAGS=${GOFLAGS} -tags=sonic" >> $GITHUB_ENV
fi
shell: bash

- name: Test
run: | # PGSERVICEFILE isn't supported by lib/pq and will cause a panic if set
unset PGSERVICEFILE
Expand Down Expand Up @@ -169,4 +186,4 @@ jobs:
cd web/
npm install
npm run lint
npm run build
npm run build
14 changes: 12 additions & 2 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ linters:
- copyloopvar
# - cyclop
- decorder
# - depguard
- depguard
- dogsled
# - dupl
- dupword
Expand Down Expand Up @@ -117,6 +117,17 @@ linters:
# - zerologlint

linters-settings:
depguard:
shazbert marked this conversation as resolved.
Show resolved Hide resolved
rules:
prevent_json_package_use:
list-mode: lax # allow unless explicitely denied
files:
- $all
allow:
- github.com/thrasher-corp/gocryptotrader/encoding/json
deny:
- pkg: encoding/json
desc: "Please use the custom JSON package github.com/thrasher-corp/gocryptotrader/encoding/json instead of encoding/json to allow sonic/json switching."
govet:
enable:
- shadow
Expand All @@ -141,7 +152,6 @@ linters-settings:
- float-compare
# We deliberately use Equal over Len to avoid spamming the contents of large Slices
- len

issues:
max-issues-per-linter: 0
max-same-issues: 0
Expand Down
2 changes: 1 addition & 1 deletion LICENSE
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
The MIT License (MIT)

Copyright (c) 2014-2024 The GoCryptoTrader Developers
Copyright (c) 2014-2025 The GoCryptoTrader Developers
gbjk marked this conversation as resolved.
Show resolved Hide resolved

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
Expand Down
4 changes: 4 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,7 @@ endef
check-jq:
@printf "Checking if jq is installed... "
@command -v jq >/dev/null 2>&1 && { printf "OK\n"; } || { printf "FAILED. Please install jq to proceed.\n"; exit 1; }

.PHONY: sonic
sonic:
go build -tags=sonic ./...
Comment on lines +82 to +84
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is different from all previous examples.
You're not building sonic. You're building gct with sonic.
And it leaves a gap where install should be.

I suggest that users need to use make build and install with an argument:

make install JSON=sonic

Or, if we go the route I'm proposing:

make install JSON=golang

Also: Given the recent highlighting of act, I wonder if we remove Makefile and update docs to use act, and make that a (non-runtime) dependency for installing GCT from source.
Note: As we approach semver (right?), we should be issuing binaries anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we are going to allow customisation, I think a more general approach make build FLAGS="-tags=sonic" might be better. This gives users the flexibility to specify any build tags or flags they need without hardcoding specific options in the Makefile. If users have to manually specify flags every time, they might as well use go build or go install directly. The goal of make is to simplify workflows, not add unnecessary complexity.

We could:

Keep the Makefile but allow FLAGS to be overridden for advanced use cases.
Provide default targets like make build-sonic for common configurations.

As for act that is an option. But I have little to no experience in semver and issuing binaries so you can discuss with @thrasher- && @gloriousCode

Copy link
Collaborator

@gbjk gbjk Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If you're going to use FLAGS, then:

GOFLAGS="-tags=hedgehogless" make build

makes more sense than adding it to the end, and means no extra support for FLAGS in Makefile.
( Note: I'll accept nosonic or sonicless )

act: I've only just started using it

Semver is a separate topic, and issuing binaries doesn't need to be related to it, now that I think about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Can keep the Makefile, as I find other recipes useful locally like pprof and only use act for simulating GHA workflows which I don't think it can do that also. We also want to keep the number of dependencies (runtime and non-runtime) down as much as possible for GCT ouf of the box.
  2. I'm happy with supporting both options who want a quick shorthand recipe and make install/make build param for power users (we do this with LTC where things are diisabled by NO_X).

11 changes: 6 additions & 5 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ However, we welcome pull requests for any exchange which does not match this cri
+ WebGUI (discontinued).
+ Exchange HTTP mock testing. See [mock](/exchanges/mock/README.md).
+ Exchange multichain deposits and withdrawals for specific exchanges. See [multichain transfer support](/docs/MULTICHAIN_TRANSFER_SUPPORT.md).
+ Sonic JSON Integration. Swap between the default Go 'encoding/json' package and the sonic library using go build tags for optimized JSON handling `-tags=sonic` or by using the make file `make sonic`

## Development Tracking

Expand Down Expand Up @@ -148,15 +149,15 @@ Binaries will be published once the codebase reaches a stable condition.

|User|Contribution Amount|
|--|--|
| [thrasher-](https://github.com/thrasher-) | 703 |
| [shazbert](https://github.com/shazbert) | 355 |
| [dependabot[bot]](https://github.com/apps/dependabot) | 331 |
| [thrasher-](https://github.com/thrasher-) | 704 |
| [shazbert](https://github.com/shazbert) | 356 |
| [dependabot[bot]](https://github.com/apps/dependabot) | 344 |
| [gloriousCode](https://github.com/gloriousCode) | 236 |
| [gbjk](https://github.com/gbjk) | 107 |
| [gbjk](https://github.com/gbjk) | 111 |
| [dependabot-preview[bot]](https://github.com/apps/dependabot-preview) | 88 |
| [xtda](https://github.com/xtda) | 47 |
| [lrascao](https://github.com/lrascao) | 27 |
| [Beadko](https://github.com/Beadko) | 17 |
| [Beadko](https://github.com/Beadko) | 18 |
| [Rots](https://github.com/Rots) | 15 |
| [vazha](https://github.com/vazha) | 15 |
| [ydm](https://github.com/ydm) | 15 |
Expand Down
2 changes: 1 addition & 1 deletion backtester/btcli/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package main

import (
"context"
"encoding/json"
"fmt"
"log"
"os"
Expand All @@ -13,6 +12,7 @@ import (

"github.com/thrasher-corp/gocryptotrader/common"
"github.com/thrasher-corp/gocryptotrader/core"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
"github.com/thrasher-corp/gocryptotrader/gctrpc/auth"
"github.com/thrasher-corp/gocryptotrader/signaler"
"github.com/urfave/cli/v2"
Expand Down
2 changes: 1 addition & 1 deletion backtester/config/backtesterconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
package config

import (
"encoding/json"
"fmt"
"os"
"path/filepath"

"github.com/thrasher-corp/gocryptotrader/backtester/common"
"github.com/thrasher-corp/gocryptotrader/common/file"
gctconfig "github.com/thrasher-corp/gocryptotrader/config"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
)

// ReadBacktesterConfigFromPath will take a config from a path
Expand Down
2 changes: 1 addition & 1 deletion backtester/config/batcktesterconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
package config

import (
"encoding/json"
"errors"
"path/filepath"
"testing"

"github.com/thrasher-corp/gocryptotrader/backtester/common"
"github.com/thrasher-corp/gocryptotrader/common/file"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
)

func TestLoadBacktesterConfig(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion backtester/config/strategyconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package config

import (
"encoding/json"
"fmt"
"os"
"strings"
Expand All @@ -13,6 +12,7 @@ import (
"github.com/thrasher-corp/gocryptotrader/backtester/eventhandlers/strategies/base"
gctcommon "github.com/thrasher-corp/gocryptotrader/common"
"github.com/thrasher-corp/gocryptotrader/common/file"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
"github.com/thrasher-corp/gocryptotrader/exchanges/asset"
"github.com/thrasher-corp/gocryptotrader/log"
)
Expand Down
2 changes: 1 addition & 1 deletion backtester/config/strategyconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package config

import (
"encoding/json"
"errors"
"os"
"path/filepath"
Expand All @@ -17,6 +16,7 @@ import (
"github.com/thrasher-corp/gocryptotrader/currency"
"github.com/thrasher-corp/gocryptotrader/database"
"github.com/thrasher-corp/gocryptotrader/database/drivers"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
"github.com/thrasher-corp/gocryptotrader/exchanges/account"
"github.com/thrasher-corp/gocryptotrader/exchanges/asset"
"github.com/thrasher-corp/gocryptotrader/exchanges/kline"
Expand Down
2 changes: 1 addition & 1 deletion backtester/config/strategyconfigbuilder/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package main

import (
"bufio"
"encoding/json"
"errors"
"fmt"
"log"
Expand All @@ -23,6 +22,7 @@ import (
"github.com/thrasher-corp/gocryptotrader/database"
dbPSQL "github.com/thrasher-corp/gocryptotrader/database/drivers/postgres"
dbsqlite3 "github.com/thrasher-corp/gocryptotrader/database/drivers/sqlite3"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
"github.com/thrasher-corp/gocryptotrader/exchanges/asset"
gctkline "github.com/thrasher-corp/gocryptotrader/exchanges/kline"
)
Expand Down
2 changes: 1 addition & 1 deletion backtester/eventhandlers/statistics/statistics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package statistics

import (
"encoding/json"
"fmt"
"time"

Expand All @@ -17,6 +16,7 @@ import (
gctcommon "github.com/thrasher-corp/gocryptotrader/common"
"github.com/thrasher-corp/gocryptotrader/common/key"
"github.com/thrasher-corp/gocryptotrader/currency"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
"github.com/thrasher-corp/gocryptotrader/log"
)

Expand Down
2 changes: 1 addition & 1 deletion backtester/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package main

import (
"encoding/json"
"flag"
"fmt"
"net/http"
Expand All @@ -15,6 +14,7 @@ import (
"github.com/thrasher-corp/gocryptotrader/backtester/plugins/strategies"
"github.com/thrasher-corp/gocryptotrader/common/convert"
"github.com/thrasher-corp/gocryptotrader/common/file"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
"github.com/thrasher-corp/gocryptotrader/engine"
"github.com/thrasher-corp/gocryptotrader/log"
"github.com/thrasher-corp/gocryptotrader/signaler"
Expand Down
2 changes: 1 addition & 1 deletion cmd/apichecker/apicheck.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package main

import (
"context"
"encoding/json"
"errors"
"flag"
"fmt"
Expand All @@ -18,6 +17,7 @@ import (

"github.com/thrasher-corp/gocryptotrader/common"
gctfile "github.com/thrasher-corp/gocryptotrader/common/file"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
exchange "github.com/thrasher-corp/gocryptotrader/exchanges"
"github.com/thrasher-corp/gocryptotrader/exchanges/request"
"github.com/thrasher-corp/gocryptotrader/log"
Expand Down
2 changes: 1 addition & 1 deletion cmd/apichecker/apicheck_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
package main

import (
"encoding/json"
"os"
"reflect"
"testing"

gctfile "github.com/thrasher-corp/gocryptotrader/common/file"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
exchange "github.com/thrasher-corp/gocryptotrader/exchanges"
"github.com/thrasher-corp/gocryptotrader/log"
)
Expand Down
2 changes: 1 addition & 1 deletion cmd/config_builder/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ package main

import (
"context"
"encoding/json"
"log"
"sync"

"github.com/thrasher-corp/gocryptotrader/config"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
"github.com/thrasher-corp/gocryptotrader/engine"
exchange "github.com/thrasher-corp/gocryptotrader/exchanges"
)
Expand Down
2 changes: 1 addition & 1 deletion cmd/documentation/documentation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package main

import (
"context"
"encoding/json"
"errors"
"flag"
"fmt"
Expand All @@ -17,6 +16,7 @@ import (
"github.com/thrasher-corp/gocryptotrader/common"
"github.com/thrasher-corp/gocryptotrader/common/file"
"github.com/thrasher-corp/gocryptotrader/core"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
"golang.org/x/text/cases"
"golang.org/x/text/language"
)
Expand Down
1 change: 1 addition & 0 deletions cmd/documentation/root_templates/root_readme.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ However, we welcome pull requests for any exchange which does not match this cri
+ WebGUI (discontinued).
+ Exchange HTTP mock testing. See [mock](/exchanges/mock/README.md).
+ Exchange multichain deposits and withdrawals for specific exchanges. See [multichain transfer support](/docs/MULTICHAIN_TRANSFER_SUPPORT.md).
+ Sonic JSON Integration. Swap between the default Go 'encoding/json' package and the sonic library using go build tags for optimized JSON handling `-tags=sonic` or by using the make file `make sonic`

## Development Tracking

Expand Down
4 changes: 2 additions & 2 deletions cmd/exchange_wrapper_issues/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package main

import (
"context"
"encoding/json"
"errors"
"flag"
"fmt"
Expand All @@ -19,6 +18,7 @@ import (
"github.com/thrasher-corp/gocryptotrader/common/file"
"github.com/thrasher-corp/gocryptotrader/config"
"github.com/thrasher-corp/gocryptotrader/currency"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
"github.com/thrasher-corp/gocryptotrader/engine"
exchange "github.com/thrasher-corp/gocryptotrader/exchanges"
"github.com/thrasher-corp/gocryptotrader/exchanges/account"
Expand Down Expand Up @@ -1028,7 +1028,7 @@ func testWrappers(e exchange.IBotExchange, base *exchange.Base, config *Config)
}

func jsonifyInterface(params []interface{}) json.RawMessage {
response, _ := json.MarshalIndent(params, "", " ") //nolint:errchkjson // TODO: ignore this for now
response, _ := json.MarshalIndent(params, "", " ")
return response
}

Expand Down
3 changes: 1 addition & 2 deletions cmd/exchange_wrapper_issues/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
package main

import (
"encoding/json"

"github.com/thrasher-corp/gocryptotrader/config"
"github.com/thrasher-corp/gocryptotrader/currency"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
"github.com/thrasher-corp/gocryptotrader/exchanges/asset"
)

Expand Down
2 changes: 1 addition & 1 deletion cmd/gctcli/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package main

import (
"context"
"encoding/json"
"fmt"
"log"
"os"
Expand All @@ -12,6 +11,7 @@ import (

"github.com/thrasher-corp/gocryptotrader/common"
"github.com/thrasher-corp/gocryptotrader/core"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
"github.com/thrasher-corp/gocryptotrader/exchanges/account"
"github.com/thrasher-corp/gocryptotrader/gctrpc/auth"
"github.com/thrasher-corp/gocryptotrader/signaler"
Expand Down
2 changes: 1 addition & 1 deletion cmd/gen_sqlboiler_config/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package main

import (
"encoding/json"
"flag"
"fmt"
"os"
Expand All @@ -12,6 +11,7 @@ import (
"github.com/thrasher-corp/gocryptotrader/core"
"github.com/thrasher-corp/gocryptotrader/database"
"github.com/thrasher-corp/gocryptotrader/database/repository"
"github.com/thrasher-corp/gocryptotrader/encoding/json"
)

var (
Expand Down
Loading
Loading