-
Notifications
You must be signed in to change notification settings - Fork 249
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
chore(wallet)_: community deployment related types moved to wallet requests package #6261
base: develop
Are you sure you want to change the base?
chore(wallet)_: community deployment related types moved to wallet requests package #6261
Conversation
Jenkins BuildsClick to see older builds (32)
|
1d65c1d
to
dd8d64c
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (42.30%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #6261 +/- ##
===========================================
+ Coverage 61.84% 61.87% +0.03%
===========================================
Files 843 844 +1
Lines 111287 111289 +2
===========================================
+ Hits 68823 68862 +39
+ Misses 34497 34475 -22
+ Partials 7967 7952 -15
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
LGTM
dd8d64c
to
bd9e28c
Compare
|
||
var ( | ||
ErrEmptyCollectibleName = &errors.ErrorResponse{Code: errors.ErrorCode("WRRC-001"), Details: "empty collectible name"} | ||
ErrEmptyCollectibleSymbol = &errors.ErrorResponse{Code: errors.ErrorCode("WRRC-002"), Details: "empty collectible symbol"} |
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.
Do these endpoints only apply to Collectibles? I though they were used for assets as well (since there's a Decimals member in the struct)
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.
@dlipicar yes, that's the check we had, look here: https://github.com/status-im/status-go/blob/develop/services/communitytokens/api.go#L87-L103
applicable to deployment parameters for assets and collectibles.
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.
I removed the "collectible" word from the error and made it that way more general.
bd9e28c
to
3411914
Compare
…quests package `DeploymentParameters` and `DeploymentDetails` types moved from `communitytokens` package to `requests` of the wallet service.
3411914
to
1cd6410
Compare
…outer - new file `contracts/community-tokens/contracts.go` added to unify contracts creation - the following community related path processors added: - `CommunityBurnProcessor` - `CommunityDeployAssetsProcessor` - `CommunityDeployCollectiblesProcessor` - `CommunityDeployOwnerTokenProcessor` - `CommunityMintTokensProcessor` - `CommunityRemoteBurnProcessor` - `CommunitySetSignerPubKeyProcessor` - `SendType` extended with appropriate options - added endpoints to duplicated `communitytokens` api: - `StoreDeployedCollectibles` - `StoreDeployedOwnerToken` - `StoreDeployedAssets` - removed endpoints from duplicated `communitytokens` api: - `DeployCollectibles` - `DeployOwnerToken` - `ReTrackOwnerTokenDeploymentTransaction` - `DeployAssets` - `DeployCollectiblesEstimate` - `DeployAssetsEstimate` - `DeployOwnerTokenEstimate` - `EstimateMintTokens` - `EstimateRemoteBurn` - `EstimateBurn` - `EstimateSetSignerPubKey` - `NewOwnerTokenInstance` - `NewCommunityTokenDeployerInstance` - `NewCommunityOwnerTokenRegistryInstance` - `NewCollectiblesInstance` - `NewAssetsInstance` - `MintTokens` - `RemoteBurn` - `GetCollectiblesContractInstance` - `GetAssetContractInstance` - `Burn` - `SetSignerPubKey` - `Path` type extended with new property: - `UsedContractAddress` - an address of the contract that will be used for the transaction
This is a breaking change which removes `communitytokens` package. This commit removes old `communitytokens` package, cause previously added `communitytokensv2` package, which is in use now, will be renamed to `communitytokens` in next commit.
This is a breaking change which renames `communitytokensv2` package to `communitytokens`.
Looks like you have BREAKING CHANGES in your PR. Check-list
|
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.
Looks logical to move this stuff to the wallet, thanks 👍
I would be very careful, though—could you please ask QAs to perform full regression testing for community token management (minting owner token, transferring ownership, etc.) on corresponding desktop PR before merging this?
@@ -270,7 +269,6 @@ type AssetContractData struct { | |||
|
|||
type CommunityTokensServiceInterface interface { | |||
GetCollectibleContractData(chainID uint64, contractAddress string) (*CollectibleContractData, error) | |||
SetSignerPubKey(ctx context.Context, chainID uint64, contractAddress string, txArgs wallettypes.SendTxArgs, password string, newSignerPubKey string) (string, error) |
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.
I couldn't find how the signer pubkey is set after the changes?
DeploymentParameters
andDeploymentDetails
types moved fromcommunitytokens
package torequests
of the wallet service.This is the first step in moving community-related transactions from
communitytokens
towallet
service.Related PRs: