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

US-2101 Refactor RNS registration screens for better readability and remove circular dependencies #883

Merged
merged 11 commits into from
Feb 26, 2024

Conversation

TravellerOnTheRun
Copy link
Collaborator

@jormelCoin the job for you is to test that the domain registration works as it was.

Copy link
Contributor

@jormelCoin jormelCoin left a comment

Choose a reason for hiding this comment

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

@TravellerOnTheRun, I was able to create a new RNS and also to put my old RNS.

Nonetheless, The fee transaction is wrong because is showing up as 0 TRBTC and it should be one in TRBTC and the other one in RIF like it's in production:
Screenshot 2024-02-07 at 10 09 15

Tested relay true

@TravellerOnTheRun
Copy link
Collaborator Author

@TravellerOnTheRun, I was able to create a new RNS and also to put my old RNS.

Nonetheless, The fee transaction is wrong because is showing up as 0 TRBTC and it should be one in TRBTC and the other one in RIF like it's in production: Screenshot 2024-02-07 at 10 09 15

Tested relay true

Did you compare it to recent develop?
What is shown in the explorer?

@jormelCoin
Copy link
Contributor

@TravellerOnTheRun, I was able to create a new RNS and also to put my old RNS.
Nonetheless, The fee transaction is wrong because is showing up as 0 TRBTC and it should be one in TRBTC and the other one in RIF like it's in production: Screenshot 2024-02-07 at 10 09 15
Tested relay true

Did you compare it to recent develop? What is shown in the explorer?

Yep, you're right, it shows up the RBTC. Nevertheless, in the development branch, the amount of the transaction that is deducted appears in RIF even though TRBTC appears in the browser, I assume it is because we show the fee charged in rif.

If you notice on the confirmation screen, you are not charging the 4 RIF that are charged, and they should be charged because this is the relay version, and those 4 RIF are what we should show in the transaction summary.

Evidence of this PR:

20240208_102944.mp4

Evidence of the latest build

RPReplay_Final1707401860.MP4

cc: @jessgusclark

@TravellerOnTheRun
Copy link
Collaborator Author

@TravellerOnTheRun, I was able to create a new RNS and also to put my old RNS.
Nonetheless, The fee transaction is wrong because is showing up as 0 TRBTC and it should be one in TRBTC and the other one in RIF like it's in production: Screenshot 2024-02-07 at 10 09 15
Tested relay true

Did you compare it to recent develop? What is shown in the explorer?

Yep, you're right, it shows up the RBTC. Nevertheless, in the development branch, the amount of the transaction that is deducted appears in RIF even though TRBTC appears in the browser, I assume it is because we show the fee charged in rif.

If you notice on the confirmation screen, you are not charging the 4 RIF that are charged, and they should be charged because this is the relay version, and those 4 RIF are what we should show in the transaction summary.

Evidence of this PR:

20240208_102944.mp4
Evidence of the latest build

RPReplay_Final1707401860.MP4
cc: @jessgusclark

The thing is that these issues are related to the fact how activityDeserializer works with values, since we don't have etx?.symbol it assumes RBTC, I believe there should be different ticket for that. Let's merge this one, since we have RNS working, and I'll create separate ticket for refactoring activityDeserializer, @jessgusclark what do you think?

@jormelCoin
Please, retest since I've added the actual calculation of the fee, play around with changing values.

@TravellerOnTheRun
Copy link
Collaborator Author

@TravellerOnTheRun, I was able to create a new RNS and also to put my old RNS.
Nonetheless, The fee transaction is wrong because is showing up as 0 TRBTC and it should be one in TRBTC and the other one in RIF like it's in production: Screenshot 2024-02-07 at 10 09 15
Tested relay true

Did you compare it to recent develop? What is shown in the explorer?

Yep, you're right, it shows up the RBTC. Nevertheless, in the development branch, the amount of the transaction that is deducted appears in RIF even though TRBTC appears in the browser, I assume it is because we show the fee charged in rif.
If you notice on the confirmation screen, you are not charging the 4 RIF that are charged, and they should be charged because this is the relay version, and those 4 RIF are what we should show in the transaction summary.
Evidence of this PR:
20240208_102944.mp4
Evidence of the latest build
RPReplay_Final1707401860.MP4
cc: @jessgusclark

The thing is that these issues are related to the fact how activityDeserializer works with values, since we don't have etx?.symbol it assumes RBTC, I believe there should be different ticket for that. Let's merge this one, since we have RNS working, and I'll create separate ticket for refactoring activityDeserializer, @jessgusclark what do you think?

@jormelCoin Please, retest since I've added the actual calculation of the fee, play around with changing values.

@jormelCoin @jessgusclark
Forget what I was saying, I found the way of doing it without much refactoring, but the activityDeserializer is confusing as hell :(

@jormelCoin jormelCoin added Tested Ready to move forward and removed Ready for Testing labels Feb 22, 2024
Copy link
Contributor

@jormelCoin jormelCoin left a comment

Choose a reason for hiding this comment

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

Functional tests pass

@jessgusclark jessgusclark merged commit 794c2fd into develop Feb 26, 2024
1 check passed
@jessgusclark jessgusclark deleted the us-2101 branch February 26, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tested Ready to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants