feat: burn tax exemption concept (as parameter) #52

fragwuerdig · 2023-01-18T22:01:40Z

@ZaradarBH @nghuyenthevinh2000 @inon-man @edk208 - So I made this pull request showing how we could implement the whitelist as parameter in the treasury. This is by no means complete. I would need to continue on the upgrade handling and then we would need to use the actual parameter in the ante handler. But its getting late already and I need to sleep over my cold.

Would that be good for you as a starting point?

…telist

ZaradarBH

Looks good. You need to sync the last changes I pushed to the branch, but those should slot right in. I recon we might as well integrate it into my PR and test if altering the proto code constitutes a breaking change. By and large it shouldnt as long as we manage the "field ids" so we dont end up overlapping with legacy blocks "binary ordering", which means this might very well be a safe thing to do :)

proto/terra/treasury/v1beta1/treasury.proto

… in ante handler

@nghuyenthevinh2000

…-temp Whitelist concept parameter temp - Thank you @nghuyenthevinh2000

fragwuerdig · 2023-01-20T10:44:27Z

@nghuyenthevinh2000 I cleaned up my originally proposed parameter mess.

StrathCole · 2023-01-20T11:30:42Z

Just one thing I would like to throw in is that I am not sure if spending too much of the work on this is good at the moment. If it is for "general improvement of parameters", yes. If it's specifically for whitelisting, I would suggest not to. Because it's not even sure that any whitelisting (and what form of) will be voted on (or even pass) by governance.
Just my opinion.

fragwuerdig · 2023-01-20T12:23:23Z

Just one thing I would like to throw in is that I am not sure if spending too much of the work on this is good at the moment. If it is for "general improvement of parameters", yes. If it's specifically for whitelisting, I would suggest not to. Because it's not even sure that any whitelisting (and what form of) will be voted on (or even pass) by governance. Just my opinion.

rofl - that's exactly what I said before in the review section: here

StrathCole · 2023-01-20T13:46:21Z

rofl - that's exactly what I said before in the review section: here

Thanks, didn't see that.

ZaradarBH · 2023-01-20T17:54:32Z

I think in general it makes sense to prepare the code for future change via a "parameter stub" simply because its not a lot of work. So I think in the context of this PR we should just complete that work, resolve the conflicts and get the PR merged :)

ZaradarBH · 2023-01-25T08:54:46Z

Great work @nghuyenthevinh2000 & @fragwuerdig :)

custom/auth/ante/expected_keeper.go

nghuyenthevinh2000 · 2023-01-25T15:01:06Z

@fragwuerdig @JoowonYun
I would merge this now, do you have any further comments?

x/treasury/keeper/migrations.go

ZaradarBH · 2023-01-26T09:03:36Z

The liveness test seems to keep failing on this PR :) @nghuyenthevinh2000 can you look into what might be breaking this?

x/treasury/client/cli/gov_tx.go

JoowonYun

SetWhitelistAddressProposal and RemoveWhitelistAddressProposal must be added to RegisterLegacyAminoCodec and RegisterInterfaces of the treasury module.

…t-concept-parameter

nghuyenthevinh2000 · 2023-01-31T11:50:52Z

I have run over two test:

Test Submit proposal to add whitelist address, it returns correct addresses here

Upgrade test to make sure that Binance whitelist addresses are included

Steps to reproduce

run bash scripts/whitelist-test.sh
run bash scripts/upgrade-test.sh

ZaradarBH

Great work buddy <3

JoowonYun

LGTM

ZaradarBH

I did some extended testing because quiet frankly this liveness-test failing was bugging me. If I run it locally and build the images via "make clean localnet-start" docker will build the images however when the containers are started they all instantly crash with this stack trace:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1730f7a]

goroutine 1 [running]:
github.com/cosmos/cosmos-sdk/x/gov.AppModuleBasic.RegisterRESTRoutes({{_, }, {, _, }}, {{0x0, 0x0, 0x0}, {0x2ef4308, 0xc000225a40}, ...}, ...)
github.com/cosmos/[email protected]/x/gov/module.go:79 +0x13a
github.com/cosmos/cosmos-sdk/types/module.BasicManager.RegisterRESTRoutes(...)
github.com/cosmos/[email protected]/types/module/module.go:114
github.com/terra-money/core/app.(*TerraApp).RegisterAPIRoutes(0x2edd090?, 0xc0010f2000, {0x1, 0x0, 0x0, {0xc0004dafc0, 0x12}, 0x3e8, 0xa, 0x0, ...})
github.com/terra-money/core/app/app.go:813 +0x49d
github.com/cosmos/cosmos-sdk/server.startInProcess(, {{0x0, 0x0, 0x0}, {0x2ef4308, 0xc000225a40}, {0x0, 0x0}, {0x2edd090, 0xc001204220}, ...}, ...)
github.com/cosmos/[email protected]/server/start.go:325 +0xc36
github.com/cosmos/cosmos-sdk/server.StartCmd.func2(0xc001250900?, {0xc0004e6bc0?, 0x0?, 0x4?})
github.com/cosmos/[email protected]/server/start.go:129 +0x169
github.com/spf13/cobra.(*Command).execute(0xc001250900, {0xc0004e6b80, 0x4, 0x4})
github.com/spf13/[email protected]/command.go:916 +0x862
github.com/spf13/cobra.(*Command).ExecuteC(0xc001146c00)
github.com/spf13/[email protected]/command.go:1044 +0x3bd
github.com/spf13/cobra.(*Command).Execute(...)
github.com/spf13/[email protected]/command.go:968
github.com/spf13/cobra.(*Command).ExecuteContext(...)
github.com/spf13/[email protected]/command.go:961
github.com/cosmos/cosmos-sdk/server/cmd.Execute(0x0?, {0xc00116de30, 0x7})
github.com/cosmos/[email protected]/server/cmd/execute.go:36 +0x1eb
main.main()
github.com/terra-money/core/cmd/terrad/main.go:15 +0x2c

My assumption is that something in the PR is causing the start command to blow up which is why the liveness test ultimately fails.

@nghuyenthevinh2000 if you could look into this that would be great. I will be available tomorrow between 0900 and 1200 CET to help you debug if you need assistance with the docker stuff :)

…t-concept-parameter

nghuyenthevinh2000 · 2023-01-31T17:02:22Z

I did some extended testing because quiet frankly this liveness-test failing was bugging me. If I run it locally and build the images via "make clean localnet-start" docker will build the images however when the containers are started they all instantly crash with this stack trace:

panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1730f7a]

goroutine 1 [running]: github.com/cosmos/cosmos-sdk/x/gov.AppModuleBasic.RegisterRESTRoutes({{_, }, {, _, }}, {{0x0, 0x0, 0x0}, {0x2ef4308, 0xc000225a40}, ...}, ...) github.com/cosmos/[email protected]/x/gov/module.go:79 +0x13a github.com/cosmos/cosmos-sdk/types/module.BasicManager.RegisterRESTRoutes(...) github.com/cosmos/[email protected]/types/module/module.go:114 github.com/terra-money/core/app.(*TerraApp).RegisterAPIRoutes(0x2edd090?, 0xc0010f2000, {0x1, 0x0, 0x0, {0xc0004dafc0, 0x12}, 0x3e8, 0xa, 0x0, ...}) github.com/terra-money/core/app/app.go:813 +0x49d github.com/cosmos/cosmos-sdk/server.startInProcess(, {{0x0, 0x0, 0x0}, {0x2ef4308, 0xc000225a40}, {0x0, 0x0}, {0x2edd090, 0xc001204220}, ...}, ...) github.com/cosmos/[email protected]/server/start.go:325 +0xc36 github.com/cosmos/cosmos-sdk/server.StartCmd.func2(0xc001250900?, {0xc0004e6bc0?, 0x0?, 0x4?}) github.com/cosmos/[email protected]/server/start.go:129 +0x169 github.com/spf13/cobra.(*Command).execute(0xc001250900, {0xc0004e6b80, 0x4, 0x4}) github.com/spf13/[email protected]/command.go:916 +0x862 github.com/spf13/cobra.(*Command).ExecuteC(0xc001146c00) github.com/spf13/[email protected]/command.go:1044 +0x3bd github.com/spf13/cobra.(*Command).Execute(...) github.com/spf13/[email protected]/command.go:968 github.com/spf13/cobra.(*Command).ExecuteContext(...) github.com/spf13/[email protected]/command.go:961 github.com/cosmos/cosmos-sdk/server/cmd.Execute(0x0?, {0xc00116de30, 0x7}) github.com/cosmos/[email protected]/server/cmd/execute.go:36 +0x1eb main.main() github.com/terra-money/core/cmd/terrad/main.go:15 +0x2c

My assumption is that something in the PR is causing the start command to blow up which is why the liveness test ultimately fails.

@nghuyenthevinh2000 if you could look into this that would be great. I will be available tomorrow between 0900 and 1200 CET to help you debug if you need assistance with the docker stuff :)

thanks @ZaradarBH for further debugging

it is due to me registering rest proposal handler as nil. I have changed it to an implementation

After fixing it, querying whitelist through rest api

is working here

nghuyenthevinh2000 · 2023-01-31T17:21:32Z

Ey, it passes all

fragwuerdig · 2023-01-31T18:08:23Z

Ey, it passes all

:D good job!

ZaradarBH

ZaradarTR and others added 5 commits January 18, 2023 22:47

add logic to ensure that all output addressess are matched to the whi…

c2b5384

…telist

Ad-hoc clean-up

29f8361

optimized implementation

93c7f36

make make proto-gen work

0e148f1

add whitelist to treasury params and regenerate proto

d1ae55d

fragwuerdig requested review from ZaradarBH, inon-man and nghuyenthevinh2000 January 18, 2023 22:07

ZaradarBH reviewed Jan 18, 2023

View reviewed changes

Merge 'whitelist-concept' into 'whitelist-concept-parameter' to sync up

31e0d90

JoowonYun reviewed Jan 19, 2023

View reviewed changes

proto/terra/treasury/v1beta1/treasury.proto Outdated Show resolved Hide resolved

fragwuerdig and others added 7 commits January 19, 2023 12:49

treasury module keeper getters/setters for tax whitelist + using them…

8daadad

… in ante handler

add gov proto

3747798

add whitelist address substore and proposal

0b90628

update burntax to use substore check for whitelist address

e090d50

add client handler for gov

d1d60ed

Merge pull request #55 from classic-terra/whitelist-concept-parameter…

7a52f4b

…-temp Whitelist concept parameter temp - Thank you @nghuyenthevinh2000

cleanup of originally proposed whitelist parameter code

8df2d95

Base automatically changed from whitelist-concept to main January 20, 2023 14:10

ZaradarBH assigned fragwuerdig Jan 20, 2023

ZaradarBH added documentation Improvements or additions to documentation out of scope work that is unapproved by the community, but still essential for the L1 team enhancement New feature or request and removed documentation Improvements or additions to documentation labels Jan 20, 2023

ZaradarBH added this to the v1.0.6 milestone Jan 20, 2023

ZaradarBH approved these changes Jan 25, 2023

View reviewed changes

custom/auth/ante/expected_keeper.go Show resolved Hide resolved

JoowonYun reviewed Jan 26, 2023

View reviewed changes

x/treasury/keeper/migrations.go Show resolved Hide resolved

JoowonYun reviewed Jan 26, 2023

View reviewed changes

x/treasury/client/cli/gov_tx.go Show resolved Hide resolved

JoowonYun suggested changes Jan 26, 2023

View reviewed changes

nghuyenthevinh2000 added 3 commits January 31, 2023 14:13

Merge branch 'main' of github.com:classic-terra/classic into whitelis…

928af44

…t-concept-parameter

add prototype for whitelist query

6769e33

testing and fix for whitelist concept

09d8881

nghuyenthevinh2000 requested a review from JoowonYun January 31, 2023 11:48

nghuyenthevinh2000 requested a review from ZaradarBH January 31, 2023 12:11

ZaradarBH approved these changes Jan 31, 2023

View reviewed changes

JoowonYun approved these changes Jan 31, 2023

View reviewed changes

ZaradarBH suggested changes Jan 31, 2023

View reviewed changes

nghuyenthevinh2000 added 3 commits January 31, 2023 23:56

add code and test for querying whitelist through rest api

1a3e9b5

minor linting

fa4cdc9

Merge branch 'main' of github.com:classic-terra/classic into whitelis…

1088a5a

…t-concept-parameter

nghuyenthevinh2000 requested a review from ZaradarBH January 31, 2023 17:09

ZaradarBH approved these changes Feb 1, 2023

View reviewed changes

ZaradarBH merged commit aad3987 into main Feb 1, 2023

nghuyenthevinh2000 self-assigned this Feb 3, 2023

nghuyenthevinh2000 mentioned this pull request Feb 9, 2023

feat: add burn tax exemption list #107

Merged

8 tasks

inon-man deleted the whitelist-concept-parameter branch February 13, 2023 15:58

inon-man changed the title ~~Whitelist concept (as parameter)~~ Burn tax exemption concept (as parameter) Feb 18, 2023

inon-man changed the title ~~Burn tax exemption concept (as parameter)~~ feat: burn tax exemption concept (as parameter) Feb 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: burn tax exemption concept (as parameter) #52

feat: burn tax exemption concept (as parameter) #52

fragwuerdig commented Jan 18, 2023

ZaradarBH left a comment

fragwuerdig commented Jan 20, 2023

StrathCole commented Jan 20, 2023

fragwuerdig commented Jan 20, 2023

StrathCole commented Jan 20, 2023

ZaradarBH commented Jan 20, 2023

ZaradarBH commented Jan 25, 2023 •

edited

Loading

nghuyenthevinh2000 commented Jan 25, 2023

ZaradarBH commented Jan 26, 2023

JoowonYun left a comment

nghuyenthevinh2000 commented Jan 31, 2023

ZaradarBH left a comment

JoowonYun left a comment

ZaradarBH left a comment •

edited

Loading

nghuyenthevinh2000 commented Jan 31, 2023 •

edited

Loading

nghuyenthevinh2000 commented Jan 31, 2023

fragwuerdig commented Jan 31, 2023 •

edited

Loading

ZaradarBH left a comment

feat: burn tax exemption concept (as parameter) #52

feat: burn tax exemption concept (as parameter) #52

Conversation

fragwuerdig commented Jan 18, 2023

ZaradarBH left a comment

Choose a reason for hiding this comment

fragwuerdig commented Jan 20, 2023

StrathCole commented Jan 20, 2023

fragwuerdig commented Jan 20, 2023

StrathCole commented Jan 20, 2023

ZaradarBH commented Jan 20, 2023

ZaradarBH commented Jan 25, 2023 • edited Loading

nghuyenthevinh2000 commented Jan 25, 2023

ZaradarBH commented Jan 26, 2023

JoowonYun left a comment

Choose a reason for hiding this comment

nghuyenthevinh2000 commented Jan 31, 2023

ZaradarBH left a comment

Choose a reason for hiding this comment

JoowonYun left a comment

Choose a reason for hiding this comment

ZaradarBH left a comment • edited Loading

Choose a reason for hiding this comment

nghuyenthevinh2000 commented Jan 31, 2023 • edited Loading

nghuyenthevinh2000 commented Jan 31, 2023

fragwuerdig commented Jan 31, 2023 • edited Loading

ZaradarBH left a comment

Choose a reason for hiding this comment

ZaradarBH commented Jan 25, 2023 •

edited

Loading

ZaradarBH left a comment •

edited

Loading

nghuyenthevinh2000 commented Jan 31, 2023 •

edited

Loading

fragwuerdig commented Jan 31, 2023 •

edited

Loading