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

Jump to bottom

feat: returning correct ledger id in tokeninfo (#163) #222

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: returning correct ledger id in tokeninfo (#163) #222

wants to merge 7 commits into from

Conversation

arianejasuwienas
Copy link
Contributor

@arianejasuwienas arianejasuwienas commented Jan 28, 2025 •
edited
Loading

Description:

The Ledger ID returned in TokenInfo will now provide the correct value.

  1. In Hardhat, the Mirror Node URL will be used to determine the Ledger ID.
  2. In Foundry, we already have access to the Chain ID, so we can use it to infer the Ledger ID.

Related issue(s):

Fixes #163

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@arianejasuwienas
fix: returning correct ledger id for localnode (#163)
05905ed 
Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas@arianelabs.com>
@arianejasuwienas
fix: returning correct ledger id for localnode (#163)
8f0580f 
Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas@arianelabs.com>
@arianejasuwienas arianejasuwienas self-assigned this Jan 30, 2025
@arianejasuwienas arianejasuwienas marked this pull request as ready for review January 30, 2025 09:19
@arianejasuwienas arianejasuwienas requested a review from a team as a code owner January 30, 2025 09:19
@arianejasuwienas arianejasuwienas added the feature Enhancing an existing feature driven by business requirements. Typically backwards compatible. label Jan 30, 2025
@arianejasuwienas arianejasuwienas changed the title fix: guessing ledger id form the mirrornode url instead of always usi… fix: returning correct ledger id in tokeninfo (#163) Jan 30, 2025
@arianejasuwienas arianejasuwienas changed the title fix: returning correct ledger id in tokeninfo (#163) feat: returning correct ledger id in tokeninfo (#163) Jan 30, 2025
acuarica
acuarica reviewed Feb 4, 2025
View reviewed changes
src/forwarder/mirror-node-client.js Outdated
*
* @returns {string}
*/
getLedgerId() {
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't depend on the mirrorNodeUrl, we need to devise another approach to get the ledgerId.

@arianejasuwienas
fix: get ledger id from chain id (#163)
94819e8 
Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas@arianelabs.com>
@arianejasuwienas
fix: increasing tests timeout in order to fix github actions (#163)
021ef4b 
Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas@arianelabs.com>
@arianejasuwienas arianejasuwienas force-pushed the 163-correct-ledger-id branch 2 times, most recently from f1f4919 to 55396a8 Compare February 5, 2025 12:30
@arianejasuwienas
feat: using global variable for ledger id (#163)
bf16abf 
Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas@arianelabs.com>
acuarica
acuarica requested changes Feb 11, 2025
View reviewed changes
test/plugin/.testConfig.js Outdated
@@ -26,7 +26,7 @@ module.exports = {
*/
projectTestConfig: {
mocha: {
timeout: 30000,
timeout: 60000,
Copy link
Contributor

Choose a reason for hiding this comment

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

restore this, I found out there were some tests failing due to timeouts as well. However, we need to see where this issue is coming from. By incrementing the timeout we are just masking the issue.

src/slotmap.js Outdated
/**
* @param {number|undefined} chainId
*/
const init = chainId => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const init = chainId => {
const setLedgerId = chainId => {

src/slotmap.js Outdated
@@ -24,6 +24,16 @@ const {
storageLayout: { storage, types },
} = require('../out/HtsSystemContract.sol/HtsSystemContract.json');

let ledgerId = '0x00';
/**
* @param {number|undefined} chainId
Copy link
Contributor

Choose a reason for hiding this comment

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

where does undefined come from?

src/slotmap.js Show resolved Hide resolved
src/slotmap.js Show resolved Hide resolved
@arianejasuwienas
feat: reverting increased test times, renaming and adding doc to meth…
1540e50 
…od (#167)

Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas@arianelabs.com>
@arianejasuwienas
feat: renaming init method (#167)
714a5b0 
Signed-off-by: Mariusz Jasuwienas <mariusz.jasuwienas@arianelabs.com>
acuarica
acuarica requested changes Feb 11, 2025
View reviewed changes
src/slotmap.js Show resolved Hide resolved
src/slotmap.js
*
* @param {number} chainId
*/
const setLedgerId = chainId => {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's export this one from index.d.ts. Also we can move the doc comments there, so consumers can take advantage of the js docs.

nit: convert to function.

src/forwarder/json-rpc-forwarder.js
@@ -28,6 +28,8 @@ const { getHtsCode, getHtsStorageAt, HTSAddress, LONG_ZERO_PREFIX, getHIP719Code
/** @type {Partial<import('hardhat/types').HardhatNetworkForkingConfig>} */
const { url: forkUrl, mirrorNodeUrl, workerPort, localAddresses = [] } = workerData;

require('../slotmap').setLedgerId(workerData.chainId);
Copy link
Contributor

Choose a reason for hiding this comment

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

import setLedgerId in the require('..') that is above.

Suggested change
require('../slotmap').setLedgerId(workerData.chainId);
setLedgerId(workerData.chainId);

We should treat exported modules in declated package.json as independent. All external used functions/classes/consts should be imported from an index.js declared in package.json. This way is easier to consumers to also use the package.

src/slotmap.js
@@ -339,4 +352,4 @@ class PersistentStorageMap {
}
}

module.exports = { packValues, slotMapOf, PersistentStorageMap };
module.exports = { packValues, slotMapOf, PersistentStorageMap, setLedgerId };
Copy link
Contributor

Choose a reason for hiding this comment

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

also export setLedgerId through index.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acuarica acuarica acuarica requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@arianejasuwienas arianejasuwienas

Labels
feature Enhancing an existing feature driven by business requirements. Typically backwards compatible.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix ledgerId when forking from Local Node
2 participants
@arianejasuwienas @acuarica