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.

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

Handle withdraw from resources #1688

Merged
merged 20 commits into from
Mar 6, 2020
Merged

Handle withdraw from resources #1688

merged 20 commits into from
Mar 6, 2020

Conversation

krhubert
Copy link
Contributor

@krhubert krhubert commented Feb 27, 2020

About test with cli:

  • there is no possible way to test full path with cli, because we don't support tx (besides ownership)
  • for ownership tx cli uses keys.Keyring and cosmos still uses old Kyebase which doesn't work together
  • for cli we can only use query subcommands
  • we can test only path to create a message by running:
➜  engine git:(feature/withdraw-coins) ✗ ./bin/mesg-cli keys add test-user

- name: test-user
  type: local
  address: mesgtest1h5puw4ec89ntgzsml7uq64xc9ez996wuxxrdl8
  pubkey: mesgtestpub1addwnpepq09gg39ttn6lpg0aml9sdzvm90gdtgka5yw5arkkpn52578rfalxxgf835s
  mnemonic: ""
  threshold: 0
  pubkeys: []


**Important** write this mnemonic phrase in a safe place.
It is the only way to recover your account if you ever forget your password.

aerobic inside mail main scrub rigid wedding body seed lesson nose venue swamp chicken panther debate glow know novel horror unknown try sell build
➜  engine git:(feature/withdraw-coins) ✗ ./bin/mesg-cli tx ownership withdraw-coins AXg1nPSCGsdkzbr5N5wLq2Z5CXwxYneXFXHTQfUAUqx5 1atto --from test-user
ERROR: unknown address: account mesgtest1h5puw4ec89ntgzsml7uq64xc9ez996wuxxrdl8 does not exist

About test with rest:

+                       fmt.Fprintf(os.Stderr, "%s\n", acc.GetAddress().String())
+                       fmt.Fprintf(os.Stderr, "%s\n", testServiceHash.String())
+                       fmt.Fprintf(os.Stderr, "%s\n", sdk.AccAddress(crypto.AddressHash(testServiceHash)).String())
+                       fmt.Fprintf(os.Stderr, "sleep")
+                       time.Sleep(5 * time.Minute)

Put that in e2e tests (under "withdraw from service" tests) so it will configure all process and then you can send it with

curl -i --location --request POST '127.0.0.1:1317/ownership/withdraw-coins' \
--header 'Content-Type: application/json' \
--data-raw '{
        "amount": "700atto",
        "hash": "B8MQUF4egu9Fjfz3TuYB5ZhUtvRjhEe9XuQHAeiuo5fi",
        "base_req": {
                "memo": "glimpse upon body vast economy give taxi yellow rabbit come click ranch chronic hammer sport near rotate  charge lumber chicken cloud base thing forum",
                "account_number": "0",
                "gas": "200000",
                "sequence": "1",
                "gas_adjustment": "1.2",
                "chain_id": "mesg-dev-chain",
                "from": "mesgtest1sfjx3xmrnrllhc86jhfzdpqpckn4s69a5tpr4f"
        }
}'

You can check balances with :

curl --location --request GET '127.0.0.1:1317/bank/balances/mesgtest1sfjx3xmrnrllhc86jhfzdpqpckn4s69a5tpr4f'
curl --location --request GET '127.0.0.1:1317/bank/balances/mesgtest1vf42ul0gc39p5ml29rmvk46fesdakjuk8hs9fc'

Hum, as now I can see this tx response is not processed by network... need to debug why. In logs it occurs as recived.

@krhubert krhubert added the enhancement New feature or request label Feb 27, 2020
@krhubert krhubert added this to the next milestone Feb 27, 2020
@krhubert krhubert self-assigned this Feb 27, 2020
@krhubert krhubert force-pushed the feature/withdraw-coins branch from f2251c8 to 2943c16 Compare February 27, 2020 16:25
@krhubert krhubert force-pushed the feature/withdraw-coins branch from 2943c16 to 52a7ddc Compare February 27, 2020 18:38
@antho1404
Copy link
Member

Didn't we say that the withdraw was a resource-based handler? so like that the api is simpler for users /service/withdraw etc...?

Just to clarify, am I correct with the workflow of this feature?

  • The user wants to withdraw a token from a resource
  • Get the hash of the resource
  • Find the ownership associated with his address/resource hash
  • Call the /ownership/withdraw with the hash of the ownership

Am I right?

Isn't it easier to have something like:

  • The user wants to withdraw a token from a resource
  • Get the hash of the resource
  • Call the /[resource]/withdraw with the hash of the resource

Or

  • The user wants to withdraw a token from a resource
  • Get the hash of the resource
  • Call the /ownership/withdraw with the hash and type of the resource

@krhubert
Copy link
Contributor Author

Didn't we say that the withdraw was a resource-based handler? so like that the api is simpler for users /service/withdraw etc...?

There was 3 propositions:

Am I right?

Yep :)

Isn't it easier to have something like:

I'm not sure if I follow. In above 3 types of withdraw-coins modules we need to pass Hash and Amount so what do think is exactly easier?

the bullet you skiped Find the ownership associated with his address/resource hash has to be done every time because it validates if caller Owner is in fact the owner of the resource.

@antho1404
Copy link
Member

The part to find the ownership is required for verification but the API might not need it.
We can have either

  • /ownership/withdraw with amount, resource_type and hash (that will raise an error if you don't have ownership on this resource)
  • /service/withdraw with amount, hash (that will raise an error if you don't have ownership on this resource)

@krhubert
Copy link
Contributor Author

I'll copy-paste talk with @NicolasMahe on discord.

"/ownership/withdraw"
If we had module like bank coins etc then withdraw makes sense because it gives context
otherwise, we need to withdraw-coins (or something), because we can withdraw ownership for example and this is not what this endpoint dose.

Withdraw alone isn't related only to money.

We can have either ....

Why we need resource_type for ownership/withdraw? it's not required here at all.

So we need to choose some option (we have 3) and everyone wants to pick something else. Either someone will be out-voted or I'll just leaving it as it is, because we have functionality and we can change the api any time.

@krhubert
Copy link
Contributor Author

Add runner to ownership + withdraw on deletion. need to find a way to use client in e2e tests (not lcd server).

@NicolasMahe NicolasMahe modified the milestones: v0.19.0, next Mar 2, 2020
e2e/api_test.go Outdated Show resolved Hide resolved
@krhubert krhubert marked this pull request as ready for review March 4, 2020 07:51
Copy link
Member

@antho1404 antho1404 left a comment

Choose a reason for hiding this comment

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

How can we test it?
Please provide a description of how to test it in real conditions. Not only e2e. We need to test features manually to make sure that everything is working correctly. I think here there is some stuff missing, it looks like the CLI doesn't have the command and I'm not sure about the API.

x/ownership/internal/keeper/keeper.go Outdated Show resolved Hide resolved
e2e/execution_test.go Outdated Show resolved Hide resolved
Copy link
Member

@NicolasMahe NicolasMahe left a comment

Choose a reason for hiding this comment

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

I fixed 2 little stuff and manually test and everything seems to work 👍

@krhubert krhubert requested a review from antho1404 March 4, 2020 10:27
@NicolasMahe NicolasMahe added the release:add Pull requests that add something label Mar 4, 2020
@antho1404
Copy link
Member

antho1404 commented Mar 6, 2020

I have some issue testing it outside of the engine.

Test

Create a service

mesg-cli service:create "$(mesg-cli service:compile https://github.com/mesg-foundation/service-js-function)"

Start the service

mesg-cli service:start js-function

Create an execution:

mesg-cli service:execute js-function execute --json ~/prog/MESG/services/service-js-function/test.json

Get the ownership hash

grpcurl --proto ./protobuf/api/ownership.proto -plaintext -d "{}" localhost:50052 mesg.api.Ownership/List | jq '.ownerships' | jq '.[] | select(.resource | contains("Runner"))' | jq '.hash'

Convert this hash in base58

npm i @mesg/api then

node -e "console.log(require('@mesg/api/lib/util/base58').encode(Buffer.from('aM1cXHVZDGdV8e7WnVGQta2e9hrDO9t0h11Q1/G35LE=', 'base64')))"

Import account on the CLI

Use the same mnemonic than the one in the engine.

./bin/mesg-cli keys add -i test-account

Withdraw

./bin/mesg-cli tx ownership withdraw-coins 8472KKYT1CvtdpEXnaVAWyLwrE77mvYk1g9xxJTMoNZE 9000atto --from mesgtest1yztk7h0la9e2wdhed3h37sd50szq8hsvrvgump --chain-id mesg-dev-chain --fees 200000atto -b block

Result

code: 4
data: ""
rawlog: 'unauthorized: address mesgtest1yztk7h0la9e2wdhed3h37sd50szq8hsvrvgump is
  not owner of resource 8472KKYT1CvtdpEXnaVAWyLwrE77mvYk1g9xxJTMoNZE: failed to execute
  message; message index: 0'

Is there a bug or am I doing wrong? I'm the owner of this runner.

grpcurl --proto ./protobuf/api/ownership.proto -plaintext -d "{}" localhost:50052 mesg.api.Ownership/List | jq '.ownerships' | jq '.[] | select(.resource | contains("Runner"))'             

{
  "hash": "aM1cXHVZDGdV8e7WnVGQta2e9hrDO9t0h11Q1/G35LE=",
  "owner": "mesgtest1yztk7h0la9e2wdhed3h37sd50szq8hsvrvgump",
  "resourceHash": "awEFgRRDjf1lkGyZR93spEIq5n0P9QnFsV6cLXGO9as=",
  "resource": "Runner"
}

Edit:

This issue came from the fact that the withdraw api didn't need the owner hash like initially but the resource hash directly. I was giving the wrong hash. Now everything is fine.

@antho1404 antho1404 merged commit 101399d into dev Mar 6, 2020
@antho1404 antho1404 deleted the feature/withdraw-coins branch March 6, 2020 09:23
@NicolasMahe NicolasMahe mentioned this pull request Mar 17, 2020
"testing"

"github.com/cosmos/cosmos-sdk/crypto/keys"
"github.com/cosmos/cosmos-sdk/types/rest"
"github.com/mesg-foundation/engine/app"
Copy link

Choose a reason for hiding this comment

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

Need the payments

t.Run("withdraw from service", func(t *testing.T) {
acc, err := cclient.GetAccount()
require.NoError(t, err)
coins := sdk.NewCoins(sdk.NewCoin("atto", sdk.NewInt(7000)))
Copy link

Choose a reason for hiding this comment

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

Need coins

@@ -254,6 +256,32 @@ func testExecution(t *testing.T) {
lcdGet(t, "bank/balances/"+execAddress.String(), &coins)
require.True(t, coins.AmountOf("atto").Equal(sdk.NewInt(0)))
})

t.Run("withdraw from service", func(t *testing.T) {
Copy link

Choose a reason for hiding this comment

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

Need to put on go 2bank

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request release:add Pull requests that add something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants