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

serialize payment account payload to comma delimited string #1553

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abhishek818
Copy link

fixes #1549

@abhishek818 abhishek818 requested a review from woodser as a code owner January 18, 2025 13:27
@abhishek818
Copy link
Author

abhishek818 commented Jan 18, 2025

perhaps the alternative place for this change is 'write' method of PaymentAccountTypeAdaptor class.

@woodser
Copy link
Contributor

woodser commented Jan 18, 2025

perhaps the alternative place for this change is 'write' method of PaymentAccountTypeAdaptor class.

Yeah I think the PaymentAccountTypeAdapter is actually the right place to put it, as a counterpart to reading accepted country codes and trade currencies.

Otherwise I suppose the serialization should at least be part of PaymentAccountPayload.toJson(), but the type adapter is really preferred.

@abhishek818 abhishek818 force-pushed the akg_payment_account_payload_string branch from 675bcd3 to 1af5cef Compare January 19, 2025 12:52
@abhishek818
Copy link
Author

abhishek818 commented Jan 19, 2025

#the PaymentAccountTypeAdapter is actually the right place to put it, as a counterpart to reading accepted country codes and trade currencies.

agreed, done with the changes.

@abhishek818 abhishek818 force-pushed the akg_payment_account_payload_string branch from 1af5cef to 94d3302 Compare January 19, 2025 13:02
@woodser
Copy link
Contributor

woodser commented Jan 20, 2025

The implementation looks ok, but it seems the type adapter is not being invoked when writing out json.

I tried some obvious things to register the type adapter in PaymentAccount, but those didn't work.

The solution can be tested and verified by running the test "Can validate payment account forms" in HavenoClient.test.ts.

That test is failing after the PR is applied.

@abhishek818
Copy link
Author

abhishek818 commented Jan 26, 2025

Unable to run the tests locally!
Attaching docker logs for funding image as well (responsible for port 28084, i guess)
Dont find anything from the logs :

Test Logs:

haveno-ts/docker$ sudo npm run test -- --baseCurrencyNetwork=XMR_LOCAL -t "Can get the version"

> [email protected] test
> node ./node_modules/.bin/jest --forceExit --detectOpenHandles --baseCurrencyNetwork=XMR_LOCAL -t Can get the version      

Jan-26 15:30:16:69              [L0] Initializing funding wallet
 FAIL  src/HavenoClient.test.ts (23.797 s)
  ✕ Can get the version (Test, CI) (21 ms)
  ○ skipped Can convert between XMR and atomic units (Test, CI)                                                             
  ○ skipped Can manage an account (Test, CI)                                                                                
  ..............
                                                                                                                            
  ● Can get the version (Test, CI)                                                                                          
                                                                                                                            
    Error: Request failed without response: POST http://127.0.0.1:28084/json_rpc due to underlying error:
    connect ECONNREFUSED 127.0.0.1:28084
    Error: connect ECONNREFUSED 127.0.0.1:28084

      at Function.Object.<anonymous>.AxiosError.from (node_modules/axios/lib/core/AxiosError.js:92:14)
      at RedirectableRequest.handleRequestError (node_modules/axios/lib/adapters/http.js:620:25)
      at ClientRequest.eventHandlers.<computed> (node_modules/follow-redirects/index.js:38:24)
      at Axios.request (node_modules/axios/lib/core/Axios.js:45:41)
      at node_modules/monero-ts/src/main/ts/common/MoneroRpcConnection.ts:304:20
      
      .....

Funding Docker logs:

haveno-ts/docker$ sudo docker-compose logs funding
funding_wallet  | This is the RPC monero wallet. It needs to connect to a monero
funding_wallet  | daemon to work correctly.
funding_wallet  |
funding_wallet  | Monero 'Fluorine Fermi' (v0.18.3.4-10a1b767b)
funding_wallet  | Logging to ./.localnet/monero-wallet-rpc.log
funding_wallet  | 2025-01-26 15:26:39.135       I Binding on 127.0.0.1 (IPv4):28084
funding_wallet  | 2025-01-26 15:26:41.085       W Starting wallet RPC server


@abhishek818
Copy link
Author

Trying to run it with current code first and then with my local branch changes by changing below (in dockerfile):

RUN set -ex && git clone https://github.com/haveno-dex/haveno.git /home/haveno/haveno

WORKDIR /home/haveno/haveno

RUN set -ex && git fetch origin && git checkout origin/master

@abhishek818
Copy link
Author

@woodser

@woodser
Copy link
Contributor

woodser commented Jan 27, 2025

Port 28084 is for the local monerod for the tests. Tests need to be run with make monerod1-local and make monerod2-local running: https://github.com/haveno-dex/haveno/blob/master/docs/installing.md#run-a-local-xmr-testnet

@abhishek818
Copy link
Author

tried above commands along with mining and also running funding-wallet-local make, but none worked out for running the test cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants